search

neuronsimulator / nrn

NEURON Simulator
http://nrn.readthedocs.io
Other
376 stars 113 forks source link

Header hygiene cabcode #2948

Open mgeplf opened 1 week ago

bbpbuildbot commented 1 week ago

Logfiles from GitLab pipeline #219308 (:no_entry:) have been uploaded here!

Status and direct links:

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
22 New issues
0 Accepted issues

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

See analysis details on SonarCloud

codecov[bot] commented 1 week ago

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 (8ffa1fa) to head (23cf5c0).

Files Patch % Lines
src/nrnoc/cabcode.cpp 77.77% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2948 +/- ## ======================================= Coverage 67.25% 67.26% ======================================= Files 571 571 Lines 104902 104896 -6 ======================================= + Hits 70557 70561 +4 + Misses 34345 34335 -10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

bbpbuildbot commented 1 week ago

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

Status and direct links:

mgeplf commented 1 week ago

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 commented 1 week ago

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.

mgeplf commented 1 week ago

@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: slides

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 commented 1 week ago

@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.