Skip to content

[root6] StEvent/StEnumerations.cxx fix#440

Merged
plexoos merged 23 commits intostar-bnl:mainfrom
klendathu2k:star-root6-enumerations-fix
Nov 16, 2022
Merged

[root6] StEvent/StEnumerations.cxx fix#440
plexoos merged 23 commits intostar-bnl:mainfrom
klendathu2k:star-root6-enumerations-fix

Conversation

@klendathu2k
Copy link
Copy Markdown
Contributor

@klendathu2k klendathu2k commented Nov 13, 2022

StEvent/StEnumerations.cxx uses a ROOT macro (under StRoot/macros/detectorId.C) to build a reflection map between the StDetectorId enum and the name of each enum.

Under root 5, the current code works fine. But it is ugly. We should at refactor the code to use a helper function compiled in the library. This PR makes that update.

Under root 6, the current code fails. Because root 6 tries to compile the macro, and the arguments to strncpy should not be const. Again, this PR makes detectorId a helper function w/in StEnumerations.


We move the functionality provided by StRoot/macros/detectorId.C into the compiled StEvent/StEnumerations.cxx file.

We retain StRoot/macros/detectorId.C in case one or more user macros depend on it.

klendathu2k and others added 15 commits March 11, 2022 12:29
Before submitting PR#315 some code cleanup was performed (see note below).
The fast jet directory was moved from  StarGenerator/FastJetFilt   to
StarGenerator/FILT/FastJetFilt, to conform with the standard source tree
layout for filters.

4f27034
625bbdd

Conscript-standard directory rules allow one to specify include paths
based on the module (StarGenerator) and package.  The package is one
directory beneath the module...  before code cleanup this was the
FastJetFilter directory.  After cleanup, we have to apply the fast jet
include path to all filters in the StarGenerator/FILT directory.

----

Note on the pp 200 HF filtered jet production.  The production commenced with
fast jet filter code built before code cleanup, based on the original
location of the fast jet filter in the source tree.
@klendathu2k klendathu2k marked this pull request as ready for review November 13, 2022 18:25
Comment thread StRoot/StEvent/StEnumerations.cxx Outdated
Comment thread StRoot/StEvent/StEnumerations.cxx
@plexoos
Copy link
Copy Markdown
Member

plexoos commented Nov 14, 2022

We retain StRoot/macros/detectorId.C in case one or more user macros depend on it.

I'd suggest to remove this macro entirely. It is likely that the only users of this are the Sti and Stv makers.

In the current implementation of an enum stringification in detectorId(int*, char**), it parses a header file assumed to be in a certain location and following some format then interprets the parsed lines with ROOT. Would it make sense to use a switch case instead?

@plexoos plexoos added the ROOT6 Issues and changes related to transition from ROOT5 to ROOT6 label Nov 14, 2022
Copy link
Copy Markdown
Contributor

@genevb genevb left a comment

Choose a reason for hiding this comment

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

I'm OK with a staged removal of StRoot/macros/detectorId.C if you want to call it "deprecated" for now (which should probably include that specific wording written into the macro as an easily noticed comment) and remove it at a later date. No further comments/objections from me.

@plexoos
Copy link
Copy Markdown
Member

plexoos commented Nov 15, 2022

I don't think 4789bc8 belongs here

@klendathu2k
Copy link
Copy Markdown
Contributor Author

I don't think 4789bc8 belongs here

Dang it. Pushed out on the wrong branch. How do I undo this?

@plexoos
Copy link
Copy Markdown
Member

plexoos commented Nov 15, 2022

git revert <hash>

@plexoos
Copy link
Copy Markdown
Member

plexoos commented Nov 16, 2022

Ok, I fixed the problem with compilation by reverting the changes in e82889c Nothing seems to be obviously wrong with the proposed change but it shows how fragile the original implementation is. A simple switch case for a enum to string conversion seems even more appropriate now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ROOT6 Issues and changes related to transition from ROOT5 to ROOT6

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants