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

Cython: declare pointers one per line #544

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

musicinmybrain
Copy link
Contributor

None of these declarations was incorrect, but Cython now warns about them. The developers are worried that people will write “cdef int* a, b” and think that “b” is an “int*” rather than an “int”. Reducing the number of warnings from Cython makes it easier to notice warnings that might represent real problems.

See also https://groups.google.com/g/cython-users/c/MA8I1wmoT0Q for discussion.

None of these declarations was incorrect, but Cython now warns about
them. The developers are worried that people will write “cdef int* a, b”
and think that “b” is an “int*” rather than an “int”. Reducing the
number of warnings from Cython makes it easier to notice warnings that
might represent real problems.
@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (e5e063b) 84.71% compared to head (af9fc39) 84.71%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #544   +/-   ##
=======================================
  Coverage   84.71%   84.71%           
=======================================
  Files         297      297           
  Lines       27632    27632           
  Branches     3660     3660           
=======================================
  Hits        23409    23409           
  Misses       3269     3269           
  Partials      954      954           

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

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

I doubt there's much energy for actually modifying these files, so I think reducing build noise is probably worth 100+ lines of unnecessary duplication. If people do start working on this again, it would probably be a good idea to consider/adopt Dipy's cython style guide.

Will merge sooner or later if I hear no objections.

@matthew-brett matthew-brett merged commit e39c4ef into nipy:main Nov 8, 2023
15 checks passed
@matthew-brett
Copy link
Member

Thanks - I agree with Chris, just thought I'd merge as I was here.

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 this pull request may close these issues.

None yet

3 participants