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

Redef fix #3065

Merged
merged 18 commits into from
Sep 4, 2024
Merged

Redef fix #3065

merged 18 commits into from
Sep 4, 2024

Conversation

mgeplf
Copy link
Collaborator

@mgeplf mgeplf commented Aug 30, 2024

No description provided.

@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 00c147c -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@1uc 1uc closed this Aug 30, 2024
@1uc 1uc reopened this Aug 30, 2024
@bbpbuildbot

This comment has been minimized.

Copy link

✔️ 00c147c -> Azure artifacts URL

Copy link

✔️ a4be9da -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@mgeplf
Copy link
Collaborator Author

mgeplf commented Sep 2, 2024

This helps a lot with having one less indirection, in addition, the ordering of includes is simplified, since now some code doesn't depend on having redef.h and its defines pulled in.

I think we have two options: either I can un-comment everything in redef.h, so that for backwards compat, one can still include it if there is old code that uses the old names (like: PRintf).

In my opinion, however, it would be best to have any code that is like that updated to to the "correct' names (ie: hoc_PRintf.)

Thus, I prefer the second option: delete the redef.h file.

Thoughts? @1uc / @pramodk / @alkino / @nrnhines (and whoever else should look at this)?

@1uc
Copy link
Collaborator

1uc commented Sep 2, 2024

This comes down to whether MOD files use the redef.h. If not then I think removing it would be best. If yes, then it feels like another case where we want to use NMODL to modernize MOD files automatically, e.g.

nmodl foo.mod modernize redef

to specifically modernize the redef related change. Other string would be used to modernize other things.

Copy link
Contributor

github-actions bot commented Sep 2, 2024

NEURON ModelDB CI: launching for a4be9da via its drop url

@1uc 1uc closed this Sep 2, 2024
@1uc 1uc reopened this Sep 2, 2024
@1uc 1uc closed this Sep 2, 2024
@1uc 1uc reopened this Sep 2, 2024
Copy link

✔️ a4be9da -> Azure artifacts URL

@1uc 1uc closed this Sep 2, 2024
@1uc 1uc reopened this Sep 2, 2024
Copy link

✔️ 0a4cc74 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 72.38606% with 206 lines in your changes missing coverage. Please review.

Project coverage is 67.26%. Comparing base (9373eef) to head (2e5251e).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/oc/code.cpp 73.37% 41 Missing ⚠️
src/oc/nonlin.cpp 0.00% 39 Missing ⚠️
src/oc/axis.cpp 0.00% 29 Missing ⚠️
src/oc/parse.ypp 87.43% 26 Missing ⚠️
src/oc/hoc.cpp 82.88% 19 Missing ⚠️
src/oc/fileio.cpp 77.96% 13 Missing ⚠️
src/oc/code2.cpp 47.82% 12 Missing ⚠️
src/oc/plt.cpp 0.00% 10 Missing ⚠️
src/oc/plot.cpp 16.66% 5 Missing ⚠️
src/oc/xred.cpp 0.00% 4 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3065      +/-   ##
==========================================
- Coverage   67.27%   67.26%   -0.01%     
==========================================
  Files         571      571              
  Lines      104865   104868       +3     
==========================================
- Hits        70548    70542       -6     
- Misses      34317    34326       +9     

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

Copy link
Contributor

github-actions bot commented Sep 3, 2024

NEURON ModelDB CI: launching for 0a4cc74 via its drop url

Copy link
Contributor

github-actions bot commented Sep 3, 2024

NEURON ModelDB CI: 0a4cc74 -> download reports from here

@mgeplf mgeplf marked this pull request as ready for review September 4, 2024 11:18
@1uc
Copy link
Collaborator

1uc commented Sep 4, 2024

NMODL DB CI says (unless I'm reading it incorrectly) that everything is fine.

Copy link
Collaborator

@1uc 1uc left a comment

Choose a reason for hiding this comment

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

As far as I can tell this PR carries out the renaming described in redef.h. I think this is valuable, because the defined macros have a tendency to clash with regular identifiers. It also improves consistency which is appreciated.

I feel this is a good thing to merge. @mgeplf could you please remove redef.h?

@mgeplf
Copy link
Collaborator Author

mgeplf commented Sep 4, 2024

@1uc > I feel this is a good thing to merge. @mgeplf could you please remove redef.h?

Yes, no problem; done and done.

@1uc
Copy link
Collaborator

1uc commented Sep 4, 2024

Very good! Let's give @nrnhines and @ramcdougal an opportunity to review if they wish to. There's a summary of what the PR does here:
#3065 (review)

Copy link

sonarcloud bot commented Sep 4, 2024

Copy link

✔️ 2e5251e -> Azure artifacts URL

Copy link
Member

@alkino alkino left a comment

Choose a reason for hiding this comment

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

Of course I love it !

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

Glad this was done.

@1uc 1uc merged commit 1d812c8 into master Sep 4, 2024
38 checks passed
@1uc 1uc deleted the redef_fix branch September 4, 2024 18:43
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.

5 participants