Create `FairBaseTypes.h` #1410

karabowi · 2023-06-13T10:40:04Z

Created namespace FairRoot
with struct EntryID {size_t fId;}.

Should replace PR #1405.

Checklist:

Followed the Contributing Guidelines

fairroot/tools/CMakeLists.txt

ChristianTackeGSI

I like this!

Here is a first set of comments to improve some parts.

ChristianTackeGSI · 2023-06-13T14:44:15Z

fairroot/tools/FairBaseTypes.h

+        }
+
+    private:
+        underlying_type fId;


Let's use default member initializers:

Suggested change

underlying_type fId;

underlying_type fId{std::numeric_limits<underlying_type>::max()};

ChristianTackeGSI · 2023-06-13T15:22:14Z

fairroot/tools/FairBaseTypes.h

+
+        EntryID(underlying_type value) : fId(value) {}
+
+        EntryID() : EntryID(None) {}


Which simplifies the default constructor a lot.

Suggested change

EntryID() : EntryID(None) {}

EntryID() = default;

ChristianTackeGSI · 2023-06-13T15:26:07Z

fairroot/tools/FairBaseTypes.h

+    struct EntryID {
+        using underlying_type = size_t;
+
+        static constexpr underlying_type None = std::numeric_limits<underlying_type>::max();


It would be nice, if None would be of type EntryID, right?

Suggested change

static constexpr underlying_type None = std::numeric_limits<underlying_type>::max();

static const EntryID None;

And add this after the closing } of the class, but inside the namespace:

/// \sa https://stackoverflow.com/questions/29432283/c-static-constexpr-field-with-incomplete-type/50295183#50295183 inline constexpr const EntryID EntryID::None{};

ChristianTackeGSI · 2023-06-13T15:31:37Z

fairroot/tools/FairBaseTypes.h

+
+namespace FairRoot {
+    struct EntryID {
+        using underlying_type = size_t;


I like size_t.

If you intend to use EntryId as a member of a class that gets written to a ROOT file, you should consider using an integer with a fixed size, maybe uint32_t or uint64_t, or even some of the ROOT types.

If on the other hand, EntryID is strictly for runtime only use, then size_t is fine.

ChristianTackeGSI

The first iteration of the Doxygen docs for the namespace is here. And the link to the current Doxygen build for this PR can be found here: https://github.com/FairRootGroup/FairRoot/pull/1410/checks (and click on Doxygen-Preview).

ChristianTackeGSI · 2023-06-13T15:45:00Z

fairroot/tools/FairBaseTypes.h

+namespace FairRoot {
+    struct EntryID {


Consider to add some Doxygen comments to the namespace and class?

Suggested change

namespace FairRoot {

struct EntryID {

/**

* \brief Main namespace for FairRoot classes / types

*

* All new public classes and types of FairRoot will be in this namespace.

* Note: Nost classes are still in no namespace.

*/

namespace FairRoot {

/**

* \brief <What Entry is this ID meant for?>

*/

struct EntryID

{

Created `namespace FairRoot` with `struct EntryID {size_t fId;}`. Should replace PR FairRootGroup#1405.

rbx · 2023-06-20T11:20:39Z

fairroot/base/steer/FairTypes.h

+ * All new public classes and types of FairRoot will be in this namespace.
+ * Note: Nost classes are still in no namespace.
+ */
+namespace FairRoot {


Didn't we agree on a lowercase name for this?

Suggested change

namespace FairRoot {

namespace fairroot {

If we didn't, I would still suggest to use lowercase. Checking the guidelines of Mozilla (upon which we base our clang-format), they seem to suggest lowercase: https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html#c-namespaces

So do the Alice guidelines: https://rawgit.com/AliceO2Group/CodingGuidelines/master/naming_formatting.html?showone=Namespace_Names#Namespace_Names

For me it's more a personal preference. But one objective reason could be so they are distinguishable from class names.

Good point, I will change it.

rbx · 2023-06-20T11:30:03Z

Soooo, what are the benefits of going full type here?

What do we loose with a simpler:

namespace fairroot
{
using EntryID = size_t;
}

rbx · 2023-06-20T11:32:28Z

fairroot/base/steer/FairTypes.h

+ * \brief Main namespace for FairRoot classes / types
+ *
+ * All new public classes and types of FairRoot will be in this namespace.
+ * Note: Nost classes are still in no namespace.


Suggested change

* Note: Nost classes are still in no namespace.

* Note: Most classes are still in no namespace.

dennisklein · 2023-06-20T12:10:48Z

Soooo, what are the benefits of going full type here?

I guess, the idea would be to have safer-to-use and better-self-documenting function signatures (and call sites), e.g.

void RunReco(EntryID start, EntryID stop); // range [start, stop]
void RunReco(EntryID start, int n);        // range [start, start + n]
void RunReco(EntryID id);                  // single entry
void RunReco(int n);                       // n entries (starting from some implicit cursor)

With an alias it would be less clear what is meant and the compiler couldn't help you, if you misuse one of the signatures. (some overloads wouldn't even be possible). Now imagine argument lists with even more int args, where some are timeouts in seconds, different ids etc, easy to mess up the order when calling.

dennisklein · 2023-06-20T12:21:21Z

@rbx But I am not 100% convinced myself, if this is the best use case for a "strong alias". Especially, as there is already existing code using all kinds of int, uint, size_t etc for the same thing including serialized root files that need to be read back etc. Not sure, how much will break for the good or the worse^^

dennisklein · 2023-06-20T12:27:44Z

fairroot/base/steer/FairRunAna.h

    /**Run for the given single entry*/
-    void Run(Long64_t entry);
-    /**Run event reconstruction from event number NStart to event number NStop */
-    void RunEventReco(Int_t NStart, Int_t NStop);


we also should deprecated the int, int overloads first before removing them, right?

Replaced `Int_t` with the new type `EntryID` in `FairRun` and derived classes. Depracated `void FairRunAna::Run(Long64_t entry)`, which internally calls introduced `void RunSingle(FairRoot::EntryID entry);`.

dennisklein · 2023-06-20T13:02:47Z

If we see a potential for more "strong aliases" like this one in the near future, we could adopt the templates proposed here: https://www.foonathan.net/2016/10/strong-typedefs/

karabowi · 2023-06-20T13:53:49Z

Soooo, what are the benefits of going full type here?

I guess, the idea would be to have safer-to-use and better-self-documenting function signatures (and call sites), e.g.
void RunReco(EntryID start, EntryID stop); // range [start, stop]
void RunReco(EntryID start, int n);        // range [start, start + n]
void RunReco(EntryID id);                  // single entry
void RunReco(int n);                       // n entries (starting from some implicit cursor)
With an alias it would be less clear what is meant and the compiler couldn't help you, if you misuse one of the signatures. (some overloads wouldn't even be possible). Now imagine argument lists with even more int args, where some are timeouts in seconds, different ids etc, easy to mess up the order when calling.

Are you also proposing to ultimately replace the Run() functions with RunReco()?

dennisklein · 2023-06-20T14:45:01Z

Soooo, what are the benefits of going full type here?

I guess, the idea would be to have safer-to-use and better-self-documenting function signatures (and call sites), e.g.
void RunReco(EntryID start, EntryID stop); // range [start, stop]
void RunReco(EntryID start, int n);        // range [start, start + n]
void RunReco(EntryID id);                  // single entry
void RunReco(int n);                       // n entries (starting from some implicit cursor)
With an alias it would be less clear what is meant and the compiler couldn't help you, if you misuse one of the signatures. (some overloads wouldn't even be possible). Now imagine argument lists with even more int args, where some are timeouts in seconds, different ids etc, easy to mess up the order when calling.
Are you also proposing to ultimately replace the Run() functions with RunReco()?

? I cannot follow

dennisklein reviewed Jun 13, 2023

View reviewed changes

fairroot/tools/CMakeLists.txt Outdated Show resolved Hide resolved

ChristianTackeGSI reviewed Jun 13, 2023

View reviewed changes

karabowi force-pushed the FairBaseTypes branch from b1796c2 to 914afe8 Compare June 19, 2023 13:29

Create FairTypes.h

600333f

Created `namespace FairRoot` with `struct EntryID {size_t fId;}`. Should replace PR FairRootGroup#1405.

karabowi force-pushed the FairBaseTypes branch from 914afe8 to 3bfc0f2 Compare June 19, 2023 14:19

rbx reviewed Jun 20, 2023

View reviewed changes

karabowi force-pushed the FairBaseTypes branch from 3bfc0f2 to 287e2b8 Compare June 20, 2023 11:23

rbx reviewed Jun 20, 2023

View reviewed changes

dennisklein reviewed Jun 20, 2023

View reviewed changes

Use EntryID type in FairRun and derived classes

b7402a6

Replaced `Int_t` with the new type `EntryID` in `FairRun` and derived classes. Depracated `void FairRunAna::Run(Long64_t entry)`, which internally calls introduced `void RunSingle(FairRoot::EntryID entry);`.

karabowi force-pushed the FairBaseTypes branch from 287e2b8 to b7402a6 Compare June 20, 2023 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `FairBaseTypes.h` #1410

Create `FairBaseTypes.h` #1410

karabowi commented Jun 13, 2023

ChristianTackeGSI left a comment

ChristianTackeGSI Jun 13, 2023

ChristianTackeGSI Jun 13, 2023

ChristianTackeGSI Jun 13, 2023

ChristianTackeGSI Jun 13, 2023

ChristianTackeGSI left a comment

ChristianTackeGSI Jun 13, 2023

rbx Jun 20, 2023

karabowi Jun 20, 2023

rbx commented Jun 20, 2023

rbx Jun 20, 2023

dennisklein commented Jun 20, 2023 •

edited

Loading

dennisklein commented Jun 20, 2023 •

edited

Loading

dennisklein Jun 20, 2023

dennisklein commented Jun 20, 2023

karabowi commented Jun 20, 2023

dennisklein commented Jun 20, 2023

	underlying_type fId;
	underlying_type fId{std::numeric_limits<underlying_type>::max()};


		EntryID(underlying_type value) : fId(value) {}

		EntryID() : EntryID(None) {}

	static constexpr underlying_type None = std::numeric_limits<underlying_type>::max();
	static const EntryID None;

	* Note: Nost classes are still in no namespace.
	* Note: Most classes are still in no namespace.

Create FairBaseTypes.h #1410

Are you sure you want to change the base?

Create FairBaseTypes.h #1410

Conversation

karabowi commented Jun 13, 2023

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristianTackeGSI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbx commented Jun 20, 2023

Choose a reason for hiding this comment

dennisklein commented Jun 20, 2023 • edited Loading

dennisklein commented Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

dennisklein commented Jun 20, 2023

karabowi commented Jun 20, 2023

dennisklein commented Jun 20, 2023

Create `FairBaseTypes.h` #1410

Create `FairBaseTypes.h` #1410

dennisklein commented Jun 20, 2023 •

edited

Loading

dennisklein commented Jun 20, 2023 •

edited

Loading