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

[BUG] Syntax error in C array declaration and initialization #5179

Open
GalaxySnail opened this issue Dec 19, 2022 · 8 comments · Fixed by #5248
Open

[BUG] Syntax error in C array declaration and initialization #5179

GalaxySnail opened this issue Dec 19, 2022 · 8 comments · Fixed by #5248

Comments

@GalaxySnail
Copy link
Contributor

Describe the bug

I found it in #5177.

def main():
    cdef int arr[4] = [1, 2, 3, 4]

This code doesn't compile:

Error compiling Cython file:
------------------------------------------------------------
...
def main():
    cdef int arr[4] = [1, 2, 3, 4]
                    ^
------------------------------------------------------------

test.pyx:2:20: Syntax error in C variable declaration

But these two do:

def main():
    cdef int arr1[4]
    arr1 = [1, 2, 3, 4]

    cdef int[4] arr2 = [1, 2, 3, 4]

I think it is already known, because it has been mentioned in the documentation:

but I couldn't find the related issue.

Anyway, if it can't be fixed easily, it will be good to document it more clearly.

Code to reproduce the behaviour:

No response

Expected behaviour

No response

Environment

Cython version: both 0.29.32 and 3.0.0a11

Additional context

No response

@matusvalo
Copy link
Contributor

matusvalo commented Jan 30, 2023

Just a thought. Should Cython support both syntaxes to declare array?

cdef int arr1[4]   # C style array declaration
cdef int[4] arr1   # Java style array declaration

it seems that both of them generate same C code but the Java style (cdef int[4] arr1) is more consistent because:

  1. Memoryviews are defined in the same way: cdef int [:, :, :] narr_view = narr [1]
  2. As mentioned in the bug report Cython currently supports cdef int[4] arr2 = [1, 2, 3, 4] but not cdef int arr[4] = [1, 2, 3, 4]
  3. Defining fused types uses syntax of TYPE[n]:
    ctypedef fused my_fused_type:
        int[3]
        float[2]

Hence my proposal is following:

Soft-deprecate C style definition - use only Java style array definition (cdef int[4] arr1) in Cython Documentation in all places but still support the C style (to keep backward compatibility).

Any thoughts?

[1] https://cython.readthedocs.io/en/latest/src/userguide/memoryviews.html

@GalaxySnail
Copy link
Contributor Author

GalaxySnail commented Jan 30, 2023

it seems that both of them generate same C code but the Java style (cdef int[4] arr1) is more consistent

Soft-deprecate C style definition - use only Java style array definition (cdef int[4] arr1) in Cython Documentation in all places but still support the C style (to keep backward compatibility).

Fair enough.

BTW, Java style cdef int[4] a, b is different from C style cdef int a[4], b (whether b is an array or an int), but the latter seems to have been deprecated for almost 10 years: 9e3a2d7 8ce5dbc.

However, the documentation said:

which will generate warnings:

warning: test.pyx:3:19: Non-trivial type declarators in shared declaration (e.g. mix of pointers and values). Each pointer declaration should be on its own line.
warning: test.pyx:3:25: Non-trivial type declarators in shared declaration (e.g. mix of pointers and values). Each pointer declaration should be on its own line.

IMO it's better to mention it in the documentation.

@da-woods
Copy link
Contributor

I'll leave this open for now. I think I'd accept a simple parser change to make the other syntax work. But I suspect it's low priority and may not happen

@williamhCode
Copy link

I was just wondering how would I declare an array of pointers with java style declaration. I stumbled across this while trying to fit some linked lists in an array.

cdef Node *data[4]
data = [NULL, NULL, NULL, NULL]

This is the only way that I can think of initialising data for now.
I was expecting it to be something like this to work:

cdef Node*[4] data = [NULL, NULL, NULL, NULL]

@scoder
Copy link
Contributor

scoder commented Apr 24, 2023 via email

@williamhCode
Copy link

williamhCode commented Apr 25, 2023

Doing cdef Node** data = [NULL, NULL, NULL, NULL] works as a replacement but it's not quite the same. One problem is if I have to define Node *data[4] in a struct or as an instance variable inside a class, then I wouldn't be able to have stack allocated array of pointers in java style syntax. Sorry for being nit picky but I feel like if Cython wants to deprecate C style syntax and move to Java style, it should support everything. Personally I'm having trouble deciding which style to use for my project due to limitations of the Java style syntax, but also C style syntax not supporting one-liners, so I end up mixing the two.

@GalaxySnail
Copy link
Contributor Author

If I understand correctly, "soft-deprecating" something doesn't mean that we will actually deprecate it. "soft-deprecated" only means it's not recommended, but will be still supported without any warnings. So if you really need the C style syntax, you can use it as expected, it's OK.

However, I agree that:

cdef Node*[4] data = [NULL, NULL, NULL, NULL]

can be a useful feature that will improve code consistency.

@matusvalo
Copy link
Contributor

"soft-deprecated" only means it's not recommended, but will be still supported without any warnings.

Yes. It should be the preferred way and should be used in all examples in documentation.

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

Successfully merging a pull request may close this issue.

5 participants