Page MenuHomeHEPForge

Rename member variables to follow convention of m_ prefix
ClosedPublic

Authored by tlatham on Mar 12 2021, 6:20 PM.

Details

Summary

Rename member variables to follow convention of m_ prefix

In addition:

  • Introduce pre-build check in the CI that the member variable naming convention is being followed in all classes
  • Fix some shadowing of variables
  • Fixes to Doxygen configuration
  • Enable shadowing and overloaded-virtual compiler warnings
Test Plan

Run tests for all models using the results of the same tests on master as reference

Diff Detail

Repository
rEVTGEN evtgen
Branch
arcpatch-D58
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 181
Build 181: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@kreps , @jback feel free to add yourselves as reviewers too
We should also decide whether to extend this to the rest of the package now or leave to later. I'd probably favour doing it all in one go.

tlatham retitled this revision from Started to remove _ to Started to rename member variables to avoid leading underscores.Mar 17 2021, 11:26 AM
tlatham edited the summary of this revision. (Show Details)
tlatham changed the visibility from "All Users" to "Public (No Login Required)".
tlatham changed the edit policy from "All Users" to "Restricted Project (Project)".
tlatham added a project: Restricted Project.
tlatham removed subscribers: jback, kreps.
kreps added a reviewer: averbyts.

I will commandeer and push rest.

  • Rename member variables starting or ending with underscore to m_xxx

I think I have it now all, but I ran to another issue with arcanist, which refused to upload diff on a large file without marking it binary. The file is src/EvtGenModels/EvtVubAC.cpp and we might need to change it back from binary when we finish this.

  • Rename preprocessor macro from _unused to UNUSED
In D58#1640, @kreps wrote:

I think I have it now all, but I ran to another issue with arcanist, which refused to upload diff on a large file without marking it binary. The file is src/EvtGenModels/EvtVubAC.cpp and we might need to change it back from binary when we finish this.

If I use arc patch D58 to apply this diff to my local repo then the file does not seem to be marked binary there.
git diff --numstat master src/EvtGenModels/EvtVubAC.cpp still tells me the number of changed lines as normal:
62814 50650 src/EvtGenModels/EvtVubAC.cpp
while I think for a binary file it would give:
- - src/EvtGenModels/EvtVubAC.cpp
So I guess it's just some internal arcanist/Differential oddness and we don't need to worry about it.

  • Rename member variables starting with underscore to m_xxx
  • Rename variables ending on underscore in EvtGenBase and small cleanup from previous chunk
  • Next part of renaming member variables finishing with underscore.
  • Rename preprocessor macro from _unused to UNUSED
  • Rename member variables starting with underscore to m_xxx
  • Rename variables ending on underscore in EvtGenBase and small cleanup from previous chunk
  • Next part of renaming member variables finishing with underscore.
  • Rename preprocessor macro from _unused to UNUSED
tlatham edited reviewers, added: kreps; removed: tlatham.
  • Fix to include external generator engines in Doxygen
  • Few more manual renames
  • Fix shadow and rename vars in EvtBBScalar
  • Remove unused var from EvtXPsiGamma
  • More renames
  • Final lot of renames
  • Fix indexing in Doxygen to ignore Evt and m_ prefixes
  • Apply clang-format
  • Fix some unwanted changes, tidy some names
  • Remove unused class EvtDecayParm
  • Reapply clang-format
  • Few additional tidy ups
  • Add pre-build checks to CI
  • Fix formatting
  • More formatting fixes after rebasing
  • Few missed renames
  • More formatting fixes
tlatham retitled this revision from Started to rename member variables to avoid leading underscores to Rename member variables to follow convention of `m_` prefix.Sep 23 2022, 5:15 PM
tlatham edited the summary of this revision. (Show Details)
tlatham edited the test plan for this revision. (Show Details)
tlatham retitled this revision from Rename member variables to follow convention of `m_` prefix to Rename member variables to follow convention of m_ prefix.

Rebase on master branch:

  • Rename member variables to avoid leading underscores
  • Enable shadowing and overloaded-virtual compiler warnings
  • Doxygen fixes
  • Add check of member variable names in CI
tlatham added a reviewer: abudinen.

I spotted a few places where we could get rid of commented code and I also have a few questions out of curiosity. Presumably, the output of the tests will be identical with the master branch. In which case, I think that this differential is ready to be landed.

EvtGenBase/EvtPDL.hh
28–30

Just a curiosity, why is this needed here?

EvtGenBase/EvtSimpleRandomEngine.hh
35

Would it make sense to use`size_t` here and in other modifications in this diff?

EvtGenModels/EvtbTosllScalarAmp.hh
31

Can we remove the commented lines in this model?

EvtGenModels/EvtbTosllVectorAmp.hh
32

Same comment here.

src/EvtGenBase/EvtDalitzReso.cpp
437–438

Similar comment here.

src/EvtGenBase/EvtFlatte.cpp
45

Also in this file there are a few lines that could be removed.

src/EvtGenBase/EvtStdHep.cpp
76

Do we need the commented code below? Looks like commented out debugging print out.

src/EvtGenModels/EvtPartWave.cpp
120

There are some variables here that look like former internal members. Should'nt we rename them??

src/EvtGenModels/EvtSLBKPoleFF.cpp
65 ↗(On Diff #484)

Some lines could be removed here?

Thanks for the check through @abudinen - I've replied to your comments inline, and will shortly push up the corresponding changes.

EvtGenBase/EvtPDL.hh
28–30

It contains the forward declaration of std::istream, which is needed for line 41

EvtGenBase/EvtSimpleRandomEngine.hh
35

While I agree that such a change is a good idea, I really prefer to keep the scope of this to the renaming of member variables to the m_-prefix convention and, related to this, fixing all instances of variable shadowing. I will open a Task to keep note of this, however, since there needs generally to be a bit of a standardisation and cleanup of the integer types used in the code.

src/EvtGenBase/EvtStdHep.cpp
76

Unclear as to why this was commented out. This function is still declared in the header as a friend function. I've uncommented it and it compiles ok, so I think I'll leave it in.

src/EvtGenModels/EvtPartWave.cpp
120

Well spotted! I've renamed them.

tlatham marked 4 inline comments as done.
  • Comments from Fernando

I have run all the tests on master and on the branch with these changes on top.

One thing that I perhaps should have drawn attention to previously, is that variable shadowing was causing a problem in both src/EvtGenModels/Evtbs2llGammaAmp.cpp and src/EvtGenModels/Evtbs2llGammaISRFSRAmp.cpp, which is now fixed in these changes.

This doesn't entirely fix the BSTOGLLISRFSR model but it does fix one problem, which is that CalcMaxProb was previously finding a maximum probability of 0.000 for the B_s0 decay cf. 0.001 for the anti-B_s0. Now the same value of 0.001 is found for both particle and anti-particle. This then removes all the Tried accept/reject: 10000 times, and rejected all the times! error messages. However, there are still (in fact, more than previously) lots of prob > probmax errors due to the value of 0.001 being too small. So, this model still needs fixing, as per EvtGen#7. The histograms for the BSTOGLLISRFSR test do change quite a lot here but it is expected given the fix applied.

In the BSTOGLLMNT model, the effect is smaller - the max prob for B_s0 changes only marginally (from 174.023 to 174.044). There are error messages of the type Tried accept/reject: 10000 times, and rejected all the times! when running both in master and with these changes. So that model also needs some further investigation. I'll note that in EvtGen#7. The changes to the histograms are very slight, as expected from the small change in the max prob value.

All other tests show no changes at all wrt the master branch in their histograms.

Thanks for the replies tlatham. I am happy with the implementations of them. Since the results of the tests look, as expected, numerically identical, except for the few cases where the changes are understood (and where the models need anyway fixing), I think that this differential is ready to be landed.

This revision is now accepted and ready to land.Apr 29 2024, 3:52 PM

Thanks @abudinen!

@kreps, @jback, @averbyts please let me know if you have any comments

Only spotted a few minor things, otherwise this is ready to be merged.

src/EvtGenBase/EvtVectorParticle.cpp
91–93

This doesn't appear to be clang-formatted. The next function is OK.

src/EvtGenExternal/EvtPHOTOS.cpp
24–25

Is the mutexing planned for this MR?

src/EvtGenModels/EvtPartWave.cpp
160

Spurious semi-colon here.

src/EvtGenExternal/EvtPHOTOS.cpp
24–25

We introduced the mutex only for PHOTOS already in D112. This is just applying the m_ rule to it.

Many thanks @jback - see replies inline.
Will push the small changes here in a second.
I'm re-running the tests just to make sure nothing changes - if all is ok I'll land this tomorrow.

src/EvtGenBase/EvtVectorParticle.cpp
91–93

I think it's the unnecessary brackets that are confusing the formatting. I'll remove them.

src/EvtGenModels/EvtPartWave.cpp
160

Well spotted! I'll remove it.

tlatham marked 2 inline comments as done.
  • Comments from John

Tests are all good, so I'll go ahead and land this.

  • Fixup changes from comments

Not sure why this wasn't automatically closed when bd48b7fe5585 was committed, so closing it manually