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.

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

Minor updates for TRestGDMLParser #202

Merged
merged 17 commits into from
May 10, 2022
Merged

Minor updates for TRestGDMLParser #202

merged 17 commits into from
May 10, 2022

Conversation

lobis
Copy link
Member

@lobis lobis commented May 9, 2022

lobis Medium: 147

TRestGDMLParser:

  • Renamed members for consistency (starting with f...)
  • Made some methods parameters by const ref.
  • When USER is not defined, will save temporary gdml file to /tmp directory instead of failing
  • Includes header to create directory recursively if not exists
  • Other minor changes

@lobis lobis requested review from jgalan, nkx111, juanangp and a team May 10, 2022 11:10
@lobis lobis marked this pull request as ready for review May 10, 2022 11:11
@@ -15,7 +15,7 @@ string(REGEX REPLACE "\n$" "" GEANT4_PATH "${GEANT4_PATH}")
set(thisGeant4 "${GEANT4_PATH}/bin/geant4.sh")

if (${REST_G4} MATCHES "ON")
set(loadG4 "\# if geant4.sh script is found we load the same Geant4 version as used in compilation\n if [[ -f \"${thisGeant4}\" && ${thisGeant4} != /usr/* ]]; then
set(loadG4 "\# if geant4.sh script is found we load the same Geant4 version as used in compilation\n if [[ -f \\\"${thisGeant4}\\\" && ${thisGeant4} != /usr/* ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

is this a bug fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really sure, it was introduced by @juanangp

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is a bug fix, it was giving an error after performing cmake


ClassDef(TRestGDMLParser, 1);
ClassDefOverride(TRestGDMLParser, 2);
Copy link
Member

Choose a reason for hiding this comment

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

For curiosity, when should we use DefOverride and why?

Copy link
Member

Choose a reason for hiding this comment

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

We should use ClassDefOverride in all the inherited functions, also we should mark all the inherited virtual functions with override, e.g.:

class Parent {
virtual void myInheritedFunction();
ClassDef(Parent, 1);
}
class Children : public Parent {
void myInheritedFunction() override;
ClassDefOverride(Children, 1);
}

In general is a good practise to mark inherited virtual functions with override, otherwise is difficult to know if the function is inherited. If ClassDefOverride is not declared, root gives a lot of warnings while labeling functions as override

I think we should update the template macros to generate different processes or metadata accordingly.

@lobis lobis merged commit 986dbc9 into master May 10, 2022
@lobis lobis deleted the lobis-activeVolume-43 branch May 12, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants