Draft: Modernisation of FileMetaData
This MR modernises implementation of some FileMetaData
-related classes, mostly by replacing function overloading with templates (making use of concepts).
It will facilitate adding more payload to FileMetaData
(e.g. from EventStreamInfo
).
/cc @gemmeren
Merge request reports
Activity
added full-integration-tests full-unit-tests labels
added EDM Tools analysis-review-required main labels
CI Result FAILURE (hash 24739fd5)Athena AthSimulation AthGeneration AnalysisBase AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 1, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 11980] (remote access info)added 28 commits
-
24739fd5...71b84775 - 27 commits from branch
atlas:main
- 3e8effb7 - Modernisation of FileMetaData
-
24739fd5...71b84775 - 27 commits from branch
added 3 commits
-
3e8effb7...1087244c - 2 commits from branch
atlas:main
- fe7e87e4 - Modernisation of FileMetaData
-
3e8effb7...1087244c - 2 commits from branch
CI Result FAILURE (hash fe7e87e4)Athena AthSimulation AthGeneration AnalysisBase AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 12049] (remote access info)added 46 commits
-
fe7e87e4...3e08e010 - 45 commits from branch
atlas:main
- 831fc41d - Modernisation of FileMetaData
-
fe7e87e4...3e08e010 - 45 commits from branch
149 case MetaDataType::simFlavour: 150 return "simFlavour"; 151 case MetaDataType::isDataOverlay: 152 return "isDataOverlay"; 153 case MetaDataType::mcCampaign: 154 return "mcCampaign"; 155 case MetaDataType::generatorsInfo: 156 return "generatorsInfo"; 157 case MetaDataType::dataYear: 158 return "dataYear"; 159 case MetaDataType::END: 160 return "END"; 161 default: 162 return "Unknown"; 163 } 164 } Unless I'm missing something,
constexpr
implies that the function is inline, so must be defined in every translation unit where it's used. In practice, I got a bunch of warnings from the clients, like this:warning: inline function 'static constexpr std::string xAOD::FileMetaData_v1::metaDataTypeToString(MetaDataType)' used but never defined
when I simply moved declaration to
.cxx
file.Do you have any suggestion regarding this?
155 case MetaDataType::generatorsInfo: 156 return "generatorsInfo"; 157 case MetaDataType::dataYear: 158 return "dataYear"; 159 case MetaDataType::END: 160 return "END"; 161 default: 162 return "Unknown"; 163 } 164 } 165 166 template <typename T, typename V> 167 requires(std::is_same_v<T, xAOD::FileMetaData_v1::MetaDataType> || 168 std::convertible_to<T, std::string_view>) && 169 FileMetaDataCompatible<V> bool 170 value(T type, V& val) const { changed this line in version 6 of the diff
Indeed,
type
is a bad name here. I renamed tometadataIdentifier
.T
is the type of the metadata identifier which can bexAOD::FileMetaData_v1::MetaDataType
orstring
-like, whereasV
is the type of the value to retrieve/set. Does it make sense?I added relevant
doxygen
documentation tovalue()
andsetValue()
functions.
I'm not the biggest fan on first look.
The finite variable types was not a shortcoming of the design. It was/is a feature. Since we can't be super flexible with the auxiliary variables that we use. (We need dictionaries for all persistent types after all.)Wouldn't it be easier to just add a couple more types to the list supported by
xAOD::FileMetaData
and be done with it?Thank you very much for the review! I appreciate the comment about the reasoning for the original design.
Since I was experimenting with adding more payload to
FileMetaData
, I found that duplicating code with more almost identical functions and macros feels unnecessary, and may be simplified with templates/concepts. But no strong opinion given your comment about the original design.What do you mean by
Wouldn't it be easier to just add a couple more types (...)
? IIUC, in current design one needs to declare for each new type the relevant accessor macro, update
FileMetaData_v1::compareWith()
, and implementvalue
/setValue
, correct?Yes, I think right now a fair amount of new code would need to be added. But we could also think of a design in which the public interface of the class is only dealing with a fixed set of types, but behind the scenes we use a bunch of templating to implement the necessary functions.
Just spit-balling here, but I could even imagine grouping all of the type-specific functions into a class like
xAOD::FileMetaDataValueAccess<T>
, and then defining the interface class like:class FileMetaData_v1 : public SG::AuxElement, public FileMetaDataValueAccess<int>, public FileMetaDataValueAccess<float>, ...
Since in such a setup we would only want to use that helper class with a finite number of template arguments, we could hide its implementation into the
Root/
directory.It's really just that I don't want our EDM to be "super templated".
That goes a bit against having an EDM in the first place. Since at one point we might as well useSG::AuxElement
as is, since that also allows us to access any type with any name in a generic way.
CI Result SUCCESS (hash dbe3bcdf)Athena AthSimulation AthGeneration AnalysisBase AthAnalysis externals cmake make tests Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 12142] (remote access info)