Improve header hygiene for cabcode.h.

[mgeplf]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.26%. Comparing base (b985a12) to head (81453ef).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/nrnoc/cabcode.cpp	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2948   +/-   ##
=======================================
  Coverage   67.26%   67.26%           
=======================================
  Files         571      571           
  Lines      104869   104863    -6     
=======================================
+ Hits        70536    70539    +3     
+ Misses      34333    34324    -9     

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

[mgeplf]

So, to me, this is an improvement, as it allows one to be more selective about includes: one doesn't need the mega nrn_ansi.h in one only needs function decls from cabcode.

The functions in cabcode.c have their decls spread over other mega-headers, and I'm on the fence of what to do with those:
https://github.com/neuronsimulator/nrn/pull/2948/files#diff-c32cab626e404e346f7562db9358ad4a389103dca70f0b628bab4d929b808529R117

I could pull out the ones in code.h, oc_ansi.h, etc, and have those include cabcode.h, which would mean the functions in cabcode.c are reflected properly in cabcode.h, but it also means those headers now include much more functionality from cabcode.c that wasn't previously available.

Thoughts @nrnhines, @pramodk, @1uc, @alkino?

[nrnhines]

This has always been a confusing issue to me. Eliminating extern from cpp files seems entirely beneficial. How to decide on partitioning for .h files is something that would benefit from best practice or principles and I'd be happy to adopt them.

[1uc]

Thank you! Just a few clarifying comment.

As for the question about "doing more", the ideal is somewhat clear, each .cpp has an associated .hpp file that declares all functions that are available to other translation units. However, for this PR, we've made enough changes.

Word-of-mouth is that this type of reorganization is a bit more error prone than expected, because there's an ABI boundary involving std::string and Python wheels. I believe it's between locally built code (nrnivmodl) and code that's part of a Python wheel and involved the issue that the wheel is built with pre-C++11 ABI and nrnivmodl builds with C++11 std::strings. Then there's also VERBATIM code in MOD files.

Hence, small gradual changes, since we might break something accidentally and then need to figure out what broke.

[mgeplf]

@nrnhines > This has always been a confusing issue to me. Eliminating extern from cpp files seems entirely beneficial. How to decide on partitioning for .h files is something that would benefit from best practice or principles and I'd be happy to adopt them.

It's a bike-shedable problem, so I don't really want to open that can of worms.
My understanding, based on the "expert"/"guru" John Lakos (who wrote Large-Scale C++ Software Design in 1996, and then the updated multi-volume Large-Scale C++) says the following has the following slides in nearly all his talks:

He has very precise explanations on what components are, and a lot of other advice that probably doesn't apply to the NEURON code base.
However, I think the above advice is sound.

Finally, I'm cognizant that NEURON is constrained by the historical/expected .h layout, so we probably can't fully implement this the above "standard", and I appreciate that.

What I think would be worthwhile would be to break out as much as possible from the *_ansi.h and other "superheaders", and then have them include the broken out decls, to maintain backwards compatibility.

[mgeplf]

@1uc > As for the question about "doing more", the ideal is somewhat clear, each .cpp has an associated .hpp file that declares all functions that are available to other translation units. However, for this PR, we've made enough changes.

Agreed.

@luc1 > Hence, small gradual changes, since we might break something accidentally and then need to figure out what broke.

And agreed.

[azure-pipelines]

✔️ abedb68 -> Azure artifacts URL

[github-actions]

NEURON ModelDB CI: launching for abedb68 via its drop url

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[azure-pipelines]

✔️ 5bbb3e2 -> Azure artifacts URL

[sonarcloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[azure-pipelines]

✔️ 81453ef -> Azure artifacts URL

[bbpbuildbot]

Logfiles from GitLab pipeline #228913 (:white_check_mark:) have been uploaded here!

Status and direct links:

• ✅ mac_m1_cmake_build: [cmake, ON, OFF, OFF, address]
• ✅ spack_setup
• ✅ build:nmodl
• ✅ build:neuron:nmodl:intel:legacy
• ✅ build:neuron:nmodl:intel:shared
• ✅ build:neuron:nmodl:nvhpc:acc:legacy
• ✅ build:neuron:nmodl:nvhpc:acc:shared
• ✅ build:neuron:nmodl:nvhpc:omp:legacy
• ✅ build:neuron:nmodl:nvhpc:omp
• ✅ test:neuron:nmodl:intel:legacy
• ✅ test:neuron:nmodl:intel:shared
• ✅ test:neuron:nmodl:nvhpc:acc:legacy
• ✅ test:neuron:nmodl:nvhpc:acc:shared
• ✅ test:neuron:nmodl:nvhpc:omp:legacy
• ✅ test:neuron:nmodl:nvhpc:omp