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

Cleanup, prepare for more C++ use. #1969

Merged
merged 9 commits into from
Sep 1, 2022
Merged

Cleanup, prepare for more C++ use. #1969

merged 9 commits into from
Sep 1, 2022

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Aug 30, 2022

A few changes hoisted out of #1929 to help manage the size of that diff.

Having common names of C++ standard library functions/members (begin, data, ...) and language keywords (delete, ...) defined as macros causes problems when more C++ headers are included. Rather than playing games with orders of includes and so on, just drop the macros and apply their old results directly to the code.

@olupton olupton marked this pull request as ready for review August 30, 2022 16:13
This interacts badly with some C++ headers.
@olupton olupton marked this pull request as draft August 30, 2022 16:36
@azure-pipelines
Copy link

✔️ 4c862db211726ff6a085562c4f6cc49202ecc59b -> Azure artifacts URL

@olupton olupton marked this pull request as ready for review August 30, 2022 17:16
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@fe1ebf0). Click here to learn what that means.
The diff coverage is 57.22%.

@@            Coverage Diff            @@
##             master    #1969   +/-   ##
=========================================
  Coverage          ?   46.49%           
=========================================
  Files             ?      526           
  Lines             ?   119259           
  Branches          ?        0           
=========================================
  Hits              ?    55445           
  Misses            ?    63814           
  Partials          ?        0           
Impacted Files Coverage Δ
src/nrncvode/cvodeobj.cpp 86.35% <0.00%> (ø)
src/nrniv/hocmech.cpp 3.39% <0.00%> (ø)
src/nrniv/impedanc.cpp 43.41% <0.00%> (ø)
src/nrniv/netpar.cpp 97.85% <ø> (ø)
src/nrnoc/nrnnemo.cpp 0.00% <ø> (ø)
src/oc/debug.cpp 0.00% <0.00%> (ø)
src/nrniv/nrnmenu.cpp 39.25% <16.66%> (ø)
src/oc/list.cpp 54.96% <20.00%> (ø)
src/nrnoc/seclist.cpp 52.96% <25.00%> (ø)
src/nrnoc/passive0.cpp 43.75% <33.33%> (ø)
... and 30 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@azure-pipelines
Copy link

✔️ 396c86c31af39d078e43cb1aedf868d470a9f6a9 -> Azure artifacts URL

@olupton
Copy link
Collaborator Author

olupton commented Aug 31, 2022

@alexsavulescu alexsavulescu marked this pull request as draft August 31, 2022 08:07
This interacts badly with some C++ headers.
This interacts badly with some C++ headers.
This interacts badly with some C++ headers, as type is a common name for
a member type. Because _type is used in some ModelDB models, the path of
least resistance is to rename type -> _type in the NEURON codebase.
@azure-pipelines
Copy link

✔️ 63e8da1 -> Azure artifacts URL

@olupton
Copy link
Collaborator Author

olupton commented Aug 31, 2022

@azure-pipelines
Copy link

✔️ 3721916 -> Azure artifacts URL

@olupton
Copy link
Collaborator Author

olupton commented Sep 1, 2022

@olupton
Copy link
Collaborator Author

olupton commented Sep 1, 2022

✔️ 3721916 -> Azure artifacts URL

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/2969304036

Looks good to me.

@olupton olupton marked this pull request as ready for review September 1, 2022 09:10
Copy link
Collaborator Author

@olupton olupton left a comment

Choose a reason for hiding this comment

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

Just flagging the few "real" changes in this PR -- the rest is "just" directly applying the result of the previous macro magic.

src/oc/redef.h Show resolved Hide resolved
src/oc/hoclist.h Show resolved Hide resolved
src/oc/hoclist.h Show resolved Hide resolved
src/oc/hoclist.h Show resolved Hide resolved
src/nrnoc/md1redef.h Show resolved Hide resolved
src/nrnoc/md1redef.h Show resolved Hide resolved
src/nrnoc/md1redef.h Show resolved Hide resolved
src/nrnoc/md1redef.h Show resolved Hide resolved
src/nrnoc/md2redef.h Show resolved Hide resolved
src/nrnoc/md2redef.h Show resolved Hide resolved
@olupton olupton merged commit 2c000b4 into master Sep 1, 2022
@olupton olupton deleted the olupton/cleanup branch September 1, 2022 09:23
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.

3 participants