Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ccall syntax to @ccall #1932

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lgoettgens
Copy link
Collaborator

@lgoettgens lgoettgens commented Oct 29, 2024

this PR was created by applying the following script to all relevant files.
I'll leave out some files to avoid conflicts with currently open PRs. To remember them for the future, the list of skipped files is:

const pattern = r"""\(?ccall\s*\(\s*\(\s*:([a-zA-Z0-9_]+)\s*,\s*([a-zA-Z0-9_]+)\s*\),\s*([a-zA-Z0-9{}_]+)\s*,\s*\(([^()]*)\)\s*,\s*(.*)\s*\)\)?"""

function replacement(str)
    if first(str) == '(' && str[end-1:end] == "))"
        str_ = str[2:end-1]
    else
        str_ = str
    end
    match = Base.match(pattern, str_)
    fun_name = match.captures[1]
    lib_name = match.captures[2]
    ret_type = match.captures[3]
    arg_types = filter(!isempty, strip.(split(match.captures[4], ",")))
    args = filter(!isempty, strip.(split(match.captures[5], ",")))
    if length(arg_types) != length(args)
        @info "Error: number of argument types and arguments do not match" fun_name lib_name ret_type arg_types args
        return str
    end
    for arg in args
        if count('(', arg) != count(')', arg)
            @info "Error: unbalanced parentheses in argument" fun_name lib_name ret_type arg_types args
            return str
        end
    end

    # Constructing new syntax for arguments
    arg_str = join(["$(maybe_parentheses(arg))::$(arg_type)" for (arg, arg_type) in zip(args, arg_types)], ", ")
    
    ret = "@ccall $(lib_name).$(fun_name)($(arg_str))::$(ret_type)"
    if str != str_
        ret = "($ret)"
    end
    return ret
end

function maybe_parentheses(arg)
    if first(arg) == '(' && last(arg) == ')'
        return arg
    end
    if occursin("+", arg) || occursin("-", arg)
        return "($arg)"
    else
        return arg
    end
end

function transform_ccalls(file::String)
    # Read the content of the input file
    content = read(file, String)

    transformed_content = replace(content, pattern => replacement)

    open(file, "w") do f
        write(f, transformed_content)
    end
end

ping @fingolfin

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 90.77135% with 201 lines in your changes missing coverage. Please review.

Project coverage is 87.38%. Comparing base (9df1993) to head (f3f0ae1).

Files with missing lines Patch % Lines
src/HeckeMoreStuff.jl 6.06% 31 Missing ⚠️
src/arb/ArbTypes.jl 61.53% 25 Missing ⚠️
src/flint/fmpz_mat.jl 75.90% 20 Missing ⚠️
src/flint/fmpq_mpoly.jl 86.90% 11 Missing ⚠️
src/flint/fmpz.jl 91.73% 10 Missing ⚠️
src/flint/fmpz_mpoly.jl 87.65% 10 Missing ⚠️
src/arb/RealPoly.jl 82.92% 7 Missing ⚠️
src/arb/Complex.jl 95.45% 6 Missing ⚠️
src/arb/acb.jl 95.38% 6 Missing ⚠️
src/arb/arb.jl 96.27% 6 Missing ⚠️
... and 22 more
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1932   +/-   ##
=======================================
  Coverage   87.38%   87.38%           
=======================================
  Files          97       97           
  Lines       35532    35533    +1     
=======================================
+ Hits        31050    31051    +1     
  Misses       4482     4482           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thofma
Copy link
Member

thofma commented Oct 29, 2024

OK, but why?

@lgoettgens
Copy link
Collaborator Author

@fingolfin already wanted to do this for some time, but hadn't found the time to do it.
My main motivation is that it makes the ccalls a lot more readable. I spent too much time in the past with trying to line up the argument types and argument values with my eyes.
We have already changed some places by hand in PRs like #1906 #1905 #1900, but doing this by hand is a lot more tedious and error-prone than just applying some regex-based rewriting once

@lgoettgens lgoettgens marked this pull request as ready for review October 29, 2024 16:24
@@ -1,2 +1,5 @@
# Adjusted indentation to 2 spaces
ba3e1355fe8b2612b1f48d26ab68f6902efde691

# Replaced many `ccall` by `@ccall`
4850d2b8e7b3c6da47836a926f727bfabe4a6654
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess for that to work we'd need to "merge" (not rebase, not squash)

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants