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.

Sign up for GitHub

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

Jump to bottom

Fix compiler warnings from MagfieldCoils #86

Merged
merged 5 commits into from
Jun 7, 2024
Merged

Fix compiler warnings from MagfieldCoils #86

merged 5 commits into from
Jun 7, 2024

Conversation

richeldichel
Copy link
Contributor

As reported in issue #84 there appear many compiler warnings when building MagfieldCoils.cxx (mostly [-Wmisleading-indentation] and [-Wunused-variable].

This pull request removes the warnings and further applies .clang-format to MagfieldCoils.cxx and MagfieldCoils.h

@richeldichel
Fix compiler warnings in MagfieldCoils.cxx
4cca8e7 
When building the class, several compiler warnings were triggered:

[build] [566/1263  43% :: 34.774] Building CXX object KEMField/Source/ExternalFields/MagfieldCoils/CMakeFiles/KEMMagfieldCoils.dir/src/MagfieldCoils.cxx.o
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::CoilRead()’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:170:58: warning: unused variable ‘v’ [-Wunused-variable]
[build]   170 |    double cu, Cx, Cy, Cz, alpha, beta, tu, L, Rmin,Rmax, v[3];
[build]       |                                                          ^
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::Magfield2EllipticCoil(int, double, double, double&, double&)’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:661:16: warning: unused variable ‘st’ [-Wunused-variable]
[build]   661 |   double sign, st, delRr, Rlow[2], Rhigh[2];
[build]       |                ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::Magsource2RemoteCoil(int, double, double)’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:873:44: warning: unused variable ‘st’ [-Wunused-variable]
[build]   873 |   double L, sigma, Zmin, Zmax, Rmin, Rmax, st;
[build]       |                                            ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::RemoteSourcepointGroup(int)’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1046:2: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
[build]  1046 |  if(zA<zmin)  zmin=zA;  if(zB>zmax)  zmax=zB;
[build]       |  ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1046:25: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
[build]  1046 |  if(zA<zmin)  zmin=zA;  if(zB>zmax)  zmax=zB;
[build]       |                         ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::MagsourceMagchargeCoils()’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1079:22: warning: variable ‘L’ set but not used [-Wunused-but-set-variable]
[build]  1079 |   double Rmin, Rmax, L, rorem, sigma;
[build]       |                      ^
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1079:25: warning: unused variable ‘rorem’ [-Wunused-variable]
[build]  1079 |   double Rmin, Rmax, L, rorem, sigma;
[build]       |                         ^~~~~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::Magsource2CentralCoil(int, double, double)’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1190:44: warning: unused variable ‘st’ [-Wunused-variable]
[build]  1190 |   double L, sigma, Zmin, Zmax, Rmin, Rmax, st;
[build]       |                                            ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::MagsourceCentralCoils()’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:10: warning: unused variable ‘z0’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |          ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:14: warning: unused variable ‘Rmin’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |              ^~~~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:20: warning: unused variable ‘Rmax’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |                    ^~~~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:26: warning: unused variable ‘L’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |                          ^
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:29: warning: unused variable ‘rorem’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |                             ^~~~~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:36: warning: unused variable ‘cu’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |                                    ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1262:40: warning: unused variable ‘tu’ [-Wunused-variable]
[build]  1262 |   double z0, Rmin, Rmax, L, rorem, cu, tu;
[build]       |                                        ^~
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx: In member function ‘void MagfieldCoils::CentralSourcepointsGroup(int)’:
[build] ../KEMField/Source/ExternalFields/MagfieldCoils/src/MagfieldCoils.cxx:1431:16: warning: unused variable ‘rocen’ [-Wunused-variable]
[build]  1431 |     double z0, rocen;
[build]       |                ^~~~~
@richeldichel
clang format MagfieldCoils.cxx to make it more readable
ab6d5ae
@richeldichel richeldichel requested a review from 2xB November 21, 2023 09:46
2xB
2xB requested changes Nov 21, 2023
View reviewed changes
Copy link
Member

@2xB 2xB left a comment •
edited by richeldichel
Loading

Choose a reason for hiding this comment

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

  • It is quite difficult to understand which variables are member variables and which are local. I would suggest refactoring member variables to the common naming scheme in Kassiopeia, being fVariableName.
  • Is it correct to return "true" in the functions if nmax is 0? Shouldn't that raise an error? Also, I think it is redundant to have an inner counting variable like int i that is followed by an outer counting variable int nmax, but that maybe doesn't matter that much.
  • Can one remove

    Kassiopeia/KEMField/Source/ExternalFields/MagfieldCoils/CMakeLists.txt

    Line 42 in 4cca8e7

    set_property(SOURCE ${MAGFIELDCOILS_SOURCEFILES} APPEND_STRING PROPERTY COMPILE_OPTIONS -Wno-error) # FIXME
    now?

@richeldichel
Copy link
Contributor Author

  • It is quite difficult to understand which variables are member variables and which are local. I would suggest refactoring member variables to the common naming scheme in Kassiopeia, being fVariableName.

    • Is it correct to return "true" in the functions if nmax is 0? Shouldn't that raise an error? Also, I think it is redundant to have an inner counting variable like int i that is followed by an outer counting variable int nmax, but that maybe doesn't matter that much.

    • Can one remove

      Kassiopeia/KEMField/Source/ExternalFields/MagfieldCoils/CMakeLists.txt

      Line 42 in 4cca8e7

      set_property(SOURCE ${MAGFIELDCOILS_SOURCEFILES} APPEND_STRING PROPERTY COMPILE_OPTIONS -Wno-error) # FIXME

      now?

Thanks for the remarks @2xB. I also see these issues. Unfortunately I also do not have the clearest overview on this code. I would propose, because I feel like these issues need a little more time than I can offer at the moment, to go ahead with this merge and put the issues onto the issue tracker.

We can, however, already remove the compile option that you mentioned!

@2xB
Copy link
Member

2xB commented May 8, 2024

Nice! I did the last remaining open point according to an answer Ferenc wrote back then (nmax <= 1 can't be used, nmax=500 is recommended, so it now throws a corresponding error in case of nmax <= 1).
Since I was a bit fast committing using the web IDE, there are two amends as separate commits, which I have to apologize for.

@2xB @richeldichel
KEMField: Throw error in case of invalid nmax
cdf420a 
Not allowing nmax <= 1 and recommending nmax=500 was suggested by Ferenc.

Fix previous commit

Fix previous commits
@richeldichel richeldichel force-pushed the fixCompilerWarnings branch from 88c313c to cdf420a Compare June 7, 2024 13:46
@richeldichel
Copy link
Contributor Author

Nice! I did the last remaining open point according to an answer Ferenc wrote back then (nmax <= 1 can't be used, nmax=500 is recommended, so it now throws a corresponding error in case of nmax <= 1). Since I was a bit fast committing using the web IDE, there are two amends as separate commits, which I have to apologize for.

Thanks! I squashed your two additional commits so that this pull request can now also be finally merged.

@richeldichel richeldichel merged commit eee1f79 into KATRIN-Experiment:main Jun 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2xB 2xB 2xB approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@richeldichel @2xB