neuronsimulator / nrn

NEURON Simulator
http://nrn.readthedocs.io
Other
381 stars 114 forks source link

clang-format problems with IFGUI...ENDGUI #2985

Closed nrnhines closed 1 month ago

nrnhines commented 1 month ago

This replaces all the IFGUI, ENDGUI macros with their expansion

if (hoc_usegui) {

and

}

respectively. The consequence is a lot of clang-format changes because there is now block indentation.

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
26 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

azure-pipelines[bot] commented 1 month ago

✔️ 69c8d49d973188ba2fb4574a4a4630900e6be591 -> Azure artifacts URL

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 44.85816% with 933 lines in your changes missing coverage. Please review.

Project coverage is 67.29%. Comparing base (3a2336e) to head (69c8d49).

Files Patch % Lines
src/ivoc/graph.cpp 49.44% 227 Missing :warning:
src/nrniv/shape.cpp 21.32% 107 Missing :warning:
src/ivoc/pwman.cpp 27.73% 99 Missing :warning:
src/nrniv/nrnmenu.cpp 14.00% 86 Missing :warning:
src/ivoc/xmenu.cpp 71.50% 55 Missing :warning:
src/ivoc/ivocvect.cpp 54.46% 51 Missing :warning:
src/ivoc/mlinedit.cpp 0.00% 45 Missing :warning:
src/ivoc/ocbox.cpp 63.06% 41 Missing :warning:
src/nrniv/shapeplt.cpp 65.97% 33 Missing :warning:
src/ivoc/ocptrvector.cpp 0.00% 32 Missing :warning:
... and 13 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2985 +/- ## ======================================= Coverage 67.29% 67.29% ======================================= Files 572 572 Lines 104928 104950 +22 ======================================= + Hits 70607 70625 +18 - Misses 34321 34325 +4 ```

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

bbpbuildbot commented 1 month ago

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

Status and direct links:

nrnhines commented 1 month ago

@pramodk

I guess replacing macro is not a bad idea anyway.

In isolation, it's good for consistent formatting. I only worry that, for a long while, people will be annoyed at the extra difficulty of merging master with existing PRs and branches. Similar to the issue when we first converted to clang-format. I did look into customizing .clang_format with

MacroBlockBegin: "..."
MacroBlockEnd: "..."

but I never figured out what the "..." should be and gathered that it would not work with an intermediate }else{ anyway. From a short term simplicity perspective, your use of

// clang-format off
...
// clang-format on

is the most direct and effective solution.

But, as one can see from 69c8d49 after the replacement, the prior formatting was quite inconsistent with the standard. If you're still in favor given this comment, let me know or go ahead and merge to master.

pramodk commented 1 month ago

If you're still in favor given this comment, let me know or go ahead and merge to master.

I have merged this already in the morning. For the few PR merges I have done since morning, conflicts were not much. So lets see if this becomes too complicated and then we can decide to revisit.