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

Avoiding conditional directives that split up parts of statements. #384

Closed
wants to merge 1 commit into from
Closed

Avoiding conditional directives that split up parts of statements. #384

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 29, 2015

A suggestion to compile entire statements and expressions, as suggested by code style guidelines from the Linux Kernel and practitioners.

It might improve code understanding, maintainability and error-proneness.

@matthew-brett
Copy link
Member

I see your point, but that particular file is auto-generated from Cython : http://cython.org - so edits will get wiped the next time we auto-generate from the nipy/labs/group/glm_twolevel.pyx file source.

You might consider sending a message to the Cython list about their auto-generated code?

@matthew-brett
Copy link
Member

Closing this one, please re-open if you disagree.

@RomeroMalaquias
Copy link

RomeroMalaquias commented Jan 13, 2017

Hello @matthew-brett i'm from the same research team of flaviommedeiros, i know that code is auto-generated, but i have to ask if you prefer this code before or after the commit. What do you think about this kind of change?
Thanks in advance.

@RomeroMalaquias
Copy link

I mean, would you accept if it was not for the fact we submitted the pull request to a code written by third parties?

@matthew-brett
Copy link
Member

The problem is that, this file is autogenerated from the Cython "pyx" file, so maintainability and readability of the C code are really not relevant - no-one should be editing this C code, because it will just get wiped out in the next rebuild of the Cython "pyx" files.

@RomeroMalaquias
Copy link

@matthew-brett ,
oh i see, but If it was not a temporary code, would you accept it?
Sorry for insisting, I'm doing a research and your response is very valuable.

@matthew-brett
Copy link
Member

I'm not a C expert, but it looks reasonable to me.

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