ROLU modified to also run with no trigger and online spectra added

[ajedele]

ROLU:
Added reader without trigger for standard R3B use.
Added notrigger external file.
Addded notrigger support for Rolu Map2Cal, Map2CalPar, Cal2Hit. Created R3BOnlineRoluSpectra.

Fixed permissions on util/c3w/build_c3w.sh

Describe your proposal.

Mention any issue this PR is resolved or is related to.

---

Checklist:

• [x ] Rebased against dev branch
• [X ] My name is in the resp. CONTRIBUTORS/AUTHORS file
• Followed the pull request guidlines and the Git workflow
• Followed the seven rules of great commit messages

[ajedele]

Dear all (mostly @YanzhaoW, @hapol, @jose-luis-rs)

Clearly, the clang-format and clang-tidy has failed. As far as I can see though, the vast majority of the issues are pointless at best.

For starters, I added an ext_h101 file, which is generated using the structify.bash script in ucesb. This file is copied from the upexps to R3BRoot in order for R3BRoot users to convert the unpacked data in the lmd files to a ROOT format that we can use. The generation of these files is from the DAQ group (@inkdot7, @hanstt, @bl0x). I do not understand why I am being flagged here. I HAVE ZERO CONTROL OVER THIS FILE!!! In addition, the DAQ group has quite a nice track record when it comes to analysis support.

Next, I have inherited this code and added features that enhance the code to allow us to use the code when a trigger is present and when it is not. This is necessary because ucesb does not generate duplicate signals. If you aren't happy with that,, you can gladly come to GSI and cable up ROLU and LOS yourself. And while you're at it, you can fix my VFTX too. Why am I being flagged for the structure of the code? I have been told that we should not use virtual with a destructor. But, IF EVERY SINCE CODE IN R3BROOT USES THIS FEATURE, WHY AM I BEING PUNISHED FOR USING YOUR CODE AND TEMPLATE!!! If you want to move away from this template, change it for every single detector, timestamp, etc. code. Don't hold me to a higher standard than you hold your current code.

There are numerous flags about 'hard-coded' values. For example, in the header files, I am initializing some variables with default values. These are being flagged. Why? You can change them in the macros. But the default should be something reasonable. The same holds for my 2 canvases. They are also being flagged for having values, not variables. It's 2(!!!) canvases.

Then there is the issue of auto. I have used the correct pointer at each place. Yet, I am being punished in the clang-tidy for knowing what the pointer is called and not just randomly assigning variables with auto.

Then there is the clang-format, which is again flagging the ext_h101 file. THE FILE I STILL HAVE NO CONTROL OVER!

It's formatting. This has no impact on whether the code compiles. Why can I not format my code how I choose to? Why am I being forced to put a space after a //. Besides being pedantically idiotic, I, personally, don't like it. I thought we could be ourselves in coding, as long as what our code compiles and shows functionality. I thought that was what makes 'R3BRoot beautiful'???

I will fix the places where the there are single letter variables. I will also fix the Int_t relics in the Reader that I oversaw. I will also work with Martin to fix issues he flags as problematic, given he actually understands c++ and the bullshit that's in this clang-tidy.

If you choose not to merge my code, fine. However, you will not have any other merge requests again from anyone. And you will have shown that pedantic idiocracy is more important to you than functional code.

Best,
Andrea

[jose-luis-rs]

Dear all (mostly @YanzhaoW, @hapol, @jose-luis-rs)

Clearly, the clang-format and clang-tidy has failed. As far as I can see though, the vast majority of the issues are pointless at best.

For starters, I added an ext_h101 file, which is generated using the structify.bash script in ucesb. This file is copied from the upexps to R3BRoot in order for R3BRoot users to convert the unpacked data in the lmd files to a ROOT format that we can use. The generation of these files is from the DAQ group (@inkdot7, @hanstt, @bl0x). I do not understand why I am being flagged here. I HAVE ZERO CONTROL OVER THIS FILE!!! In addition, the DAQ group has quite a nice track record when it comes to analysis support.

Next, I have inherited this code and added features that enhance the code to allow us to use the code when a trigger is present and when it is not. This is necessary because ucesb does not generate duplicate signals. If you aren't happy with that,, you can gladly come to GSI and cable up ROLU and LOS yourself. And while you're at it, you can fix my VFTX too. Why am I being flagged for the structure of the code? I have been told that we should not use virtual with a destructor. But, IF EVERY SINCE CODE IN R3BROOT USES THIS FEATURE, WHY AM I BEING PUNISHED FOR USING YOUR CODE AND TEMPLATE!!! If you want to move away from this template, change it for every single detector, timestamp, etc. code. Don't hold me to a higher standard than you hold your current code.

There are numerous flags about 'hard-coded' values. For example, in the header files, I am initializing some variables with default values. These are being flagged. Why? You can change them in the macros. But the default should be something reasonable. The same holds for my 2 canvases. They are also being flagged for having values, not variables. It's 2(!!!) canvases.

Then there is the issue of auto. I have used the correct pointer at each place. Yet, I am being punished in the clang-tidy for knowing what the pointer is called and not just randomly assigning variables with auto.

Then there is the clang-format, which is again flagging the ext_h101 file. THE FILE I STILL HAVE NO CONTROL OVER!

It's formatting. This has no impact on whether the code compiles. Why can I not format my code how I choose to? Why am I being forced to put a space after a //. Besides being pedantically idiotic, I, personally, don't like it. I thought we could be ourselves in coding, as long as what our code compiles and shows functionality. I thought that was what makes 'R3BRoot beautiful'???

I will fix the places where the there are single letter variables. I will also fix the Int_t relics in the Reader that I oversaw. I will also work with Martin to fix issues he flags as problematic, given he actually understands c++ and the bullshit that's in this clang-tidy.

If you choose not to merge my code, fine. However, you will not have any other merge requests again from anyone. And you will have shown that pedantic idiocracy is more important to you than functional code.

Best, Andrea

Dear Andrea

Don't worry for the ext_h101 files, they don't need to pass the clang-tidy.

About the clang-format, you only need to run the command clang-format -i name_of_files, but I don't know if this works at the GSI computers. In the case of #897 it worked fine for the clang-format ...

I am busy with teaching duties, but I will check in the weekend. I don't see any big problems to accept this PR.

Best
JL

[YanzhaoW]

TLDR:

I could see all the new code in this PR is in rolu folder, which I won't read, use nor debug. So for me personally, it's perfect fine to merge it to the dev with the current state. :D You could disable clang-tidy checking for your PR by setting up additional .clang-tidy files in your folders IF you are confident it's not necessary to fix the warnings. Should I do that for you? You could just "press a button" in the end.

Since clang-format checking is not implemented by me, you may ask other people about that.

---

Here begins my lengthy comment:

For starters, I added an ext_h101 file, which is generated using the structify.bash script in ucesb.

ucesb is written in C code and the code generated by it is also in C style. Our static analyzer checks the code according to at least C++11 standard (12 years ago, so don't call it modern). The correct way to deal with this is to exclude all those C code from clang-tidy checking, which is not difficult to implement in our CI (I will do this in few weeks).

I have been told that we should not use virtual with a destructor. But, IF EVERY SINCE CODE IN R3BROOT USES THIS FEATURE, WHY AM I BEING PUNISHED FOR USING YOUR CODE AND TEMPLATE!!!

TEMPLATE has been fixed in #894 and will land very soon.

For example, in the header files, I am initializing some variables with default values.

Code should be self-explanatory and documentation by itself.

What's the meaning of 10000 in int maxevent=10000? Can we change it? Is it fixed? Or just a pure magic number?

Then what's the meaning of DEFAULT_MAX_EVENT in

constexpr int DEFAULT_MAX_EVENT = 10000;
int maxevent = DEFAULT_MAX_EVENT;

Ok, it means the default value for maxevent because it's called so.
I know some people call this "pointless" especially when they rely on other people to tell them personally what this source file is doing instead of reading the source code line by line. Once those people leave and you have to read the source code, you would understand these "pointless" subtleties make your life much easier. And it's so easy to do. Just spend additional seconds to define it with a name in each case and it could save a lot of time for readers.

The same holds for my 2 canvases. They are also being flagged for having values, not variables. It's 2(!!!) canvases.

Same reason above. Values are just values and they don't have meanings. Named values (const or constexpr) has names to indicate their meanings.

Then there is the issue of auto. I have used the correct pointer at each place. Yet, I am being punished in the clang-tidy for knowing what the pointer is called and not just randomly assigning variables with auto.

I don't know what you mean. You may show me a specific example from your PR.

Here ends my lengthy comment

---

[ajedele]

Hi Yanzhao,

If we selectively enable and disable clang-tidy, what is the point in the clang-tidy? Shouldn't we just have a clang-tidy that flags the major issues instead of your preferred features?

All the values that are variable have respective setters with it. The values that are not variable are hard-coded. There is no foreseeable change to, for example, the number of edges we plan to read out. Also, a variable called, for example, fClockFreq is pretty straight-forward. If you don't know what that is, then it's clear you are just completely blindly calibrating the detector.

For the canvas, does this mean that I have to make a named value and give a full description of what it does? Do I need to replace it with:
const sizeofx = 800; //dimensions in x in mm
const sizeofy = 1000; //dimensions in y in mm
const sizeoftopx = 800; //Top X position of window (in pixels)
const sizeoftopy = 1000; //Top Y position of window (in pixels)
const char *name = "rolu"; //name of canvas
const char *title = "rolu"; //title of canvas
TCanvas *canvas = new TCanvas(name, title, sizeoftopx, sizeoftopy, sizeofx, sizeofy);

Just google it. The root documentation is good for this kind of thing. It's also not as demanding as you.

##[warning] use auto when initializing with a cast to avoid duplicating the type name [hicpp-use-auto,modernize-use-auto]
EXT_STR_h101_ROLU_NOTRIG_onion* data = (EXT_STR_h101_ROLU_NOTRIG_onion*)fData;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
auto

##[warning] use auto when initializing with a cast to avoid duplicating the type name [hicpp-use-auto,modernize-use-auto]
R3BRoluMappedData* hit = dynamic_cast<R3BRoluMappedData*>(fMapped->At(iHit));
^~~~~~~~~~~~~~~~~
auto

##[warning] use auto when initializing with new to avoid duplicating the type name [hicpp-use-auto,modernize-use-auto]
TFolder* mainfol = new TFolder("ROLU", "ROLU info");
^~~~~~~
auto

Best,
Andrea

[klenze]

Without invading too much into that beautiful argument you are having, I think that generally, histogramming code should be subject to less scrutiny than the main data pipeline. Basically, if histogramming code avoids out of bounds memory writes, it has already exceeded my expectations. :)

If magic constants appear in Mapped2Cal, I would be much more interested in what they mean than if they appear in some histogramming code.

For auto, I feel that both auto* cv=new TCanvas(); and TCanvas* cv=new TCanvas(); should be a-ok.

Ceterum censio: There are clang-tidy checks which should be enforced project-wide with extreme prejudice (dynamic_cast), and there are clang-tidy checks which should at most be polite suggestions (auto).

[YanzhaoW]

Hi @ajedele

Shouldn't we just have a clang-tidy that flags the major issues instead of your preferred features?

Those features aren't my preference but rather some ideas that have been accumulated over decades in programming community to make the software robust, readable and efficient (in other words, to make everyone's life easier). Each rule can be found in clang website with a clear reason why it's necessary.

If we selectively enable and disable clang-tidy, what is the point in the clang-tidy?

The point of clang-tidy is to improve the code quality of your pull request. If you are content with the bad quality code, which doesn't affect the people who hold higher standard, you could disable it. If you want to pay small extra efforts and learn how to program in a higher standard, you don't disable it.

There is no foreseeable change to, for example, the number of edges we plan to read out. Also, a variable called, for example, fClockFreq is pretty straight-forward. If you don't know what that is, then it's clear you are just completely blindly calibrating the detector.

If there is no foreseeable change to a variable, then declare it as a const.

int ClockFreq {200}; // wrong
const int ClockFreq {200}; // correct, no warning
constexpr auto ClockFreq = 200; // best way

For the canvas, does this mean that I have to make a named value and give a full description of what it does? Do I need to replace it with:
const sizeofx = 800; //dimensions in x in mm
const sizeofy = 1000; //dimensions in y in mm
const sizeoftopx = 800; //Top X position of window (in pixels)
const sizeoftopy = 1000; //Top Y position of window (in pixels)
const char *name = "rolu"; //name of canvas
const char *title = "rolu"; //title of canvas
TCanvas *canvas = new TCanvas(name, title, sizeoftopx, sizeoftopy, sizeofx, sizeofy);

This reminds me of a joke ;D:

// add x with 2
x = x + 2;

Anyway here is what I would do:

constexpr auto ROLU_CANVAS_X = 800;
constexpr auto ROLU_CANVAS_Y = 1000;
TCanvas *canvas = new TCanvas("rolu", "rolu", ROLU_CANVAS_X, ROLU_CANVAS_Y, ROLU_CANVAS_X, ROLU_CANVAS_Y);

By this you are also telling the reader that you are choosing same values for window coordinates and window sizes deliberately WITHOUT ANY COMMENT.

Then there is the issue of auto. I have used the correct pointer at each place. Yet, I am being punished in the clang-tidy for knowing what the pointer is called and not just randomly assigning variables with auto.

I see now. Then change the first type to auto. You could google the reason but I could explain here as well. The reason why this is bad:

Class1* obj = dynamic_cast<Class1*>(base_obj);

is because if someone need to change it to Class2* and he may forget changing both types, it becomes this:

Class1* obj = dynamic_cast<Class2*>(base_obj);

This gets a compilation error at best. The worst case is an implicit cast. Ok, you could say we will NEVER forget. Then tell any security or safety person in the laboratory that some rules are unnecessary because we will never forget doing something.

We could talk about these issues for days. But I don't want to bother you with these since you must have something more important to do at the moment. If it's ok to you, I will send you a pull request against this pull request to disable the clang-tidy checkings for rolu folders. In the end, you could just merge and squash.

[YanzhaoW]

Without invading too much into that beautiful argument you are having, I think that generally, histogramming code should be subject to less scrutiny than the main data pipeline. Basically, if histogramming code avoids out of bounds memory writes, it has already exceeded my expectations. :)
If magic constants appear in Mapped2Cal, I would be much more interested in what they mean than if they appear in some histogramming code.
For auto, I feel that both auto* cv=new TCanvas(); and TCanvas* cv=new TCanvas(); should be a-ok.

I basically agree with you. In some cases, magic numbers are not a big issue for the readability. But in other cases, they are a big issue, especially without any documentation (Does anyone still do documentation?). The problem is clang-tidy isn't very intelligent to differentiate those different situations. You either turn in on or off. Same goes for auto.

I would suggest to leave them on since we could have much bigger problem if they are off.

Ceterum censio: There are clang-tidy checks which should be enforced project-wide with extreme prejudice (dynamic_cast), and there are clang-tidy checks which should at most be polite suggestions (auto).

Like I said, if anyone feels clang-tidy checking is too strict, then define a separate and lesser strict setting in the corresponding folder. It's not like we have one single rule across whole project.

[klenze]

There are counterexamples for auto, though.
auto* ptr=SomeFunction();
is fine if I don't really care about the type. But it will never fail, even if the type is something vastly different than what I imagined. Instead, I will get an unrelated error message if I try to use that pointer later.

If I do not use auto, I can explicitly state my expectation of the return type:
MyType* ptr=SomeFunction();

If it compiles, than the pointer will be of the type I want.

From my understanding, C++ only allows implicit pointer casts which are safe, e.g. from a derived class to a base class. These should be fine. If I only care about the TPad properties, using TPad* ptr=new TCanvas() should be fine.

[YanzhaoW]

is fine if I don't really care about the type. But it will never fail, even if the type is something vastly different than what I imagined. Instead, I will get an unrelated error message if I try to use that pointer later.

I would say it's better to get an error (either compile or run time) if you mess something up. :D The worst case is when you mess something up, you don't get an error but rather an implicit conversion. I think the second case is the biggest reason.

From my understanding, C++ only allows implicit pointer casts which are safe, e.g. from a derived class to a base class. These should be fine. If I only care about the TPad properties, using TPad* ptr=new TCanvas() should be fine.

They are some cases that could be very toxic due to implicit conversion. For example, if Class2 has implementation inheritance from Class1 (which means the method signatures between two classes are same but the implementations are different). Then code runs fine but result is wrong.

[hapol]

Dear @ajedele,
after a PR is produced, we (all other developers) firstly see the code and we could comment and help. Thank you for producing the PR. I'm commenting on your entry:

Clearly, the clang-format and clang-tidy has failed. As far as I can see though, the vast majority of the issues are pointless at best.

Pointless issues can be discussed and discarded, accepting the PR by the release manager. Finding major errors among those possibly pointless issues is relevant and could solve severe problems. There was additional explanations by @YanzhaoW above.

This is necessary because ucesb does not generate duplicate signals. If you aren't happy with that,, you can gladly come to GSI and cable up ROLU and LOS yourself. And while you're at it, you can fix my VFTX too. Why am I being flagged for the structure of the code? I have been told that we should not use virtual with a destructor. But, IF EVERY SINCE CODE IN R3BROOT USES THIS FEATURE, WHY AM I BEING PUNISHED FOR USING YOUR CODE AND TEMPLATE!!! If you want to move away from this template, change it for every single detector, timestamp, etc. code. Don't hold me to a higher standard than you hold your current code.

As we discussed and explained previously in analysis meetings, we (all) try to include the better possible code now and (unfortunately slowly) improve the existing one when any developer has to improve or correct any existing class. As you mention we do not have enough manpower. I assume that the upper case letters correspond to shouting; the name of the branch points to me that you are not happy: but I would ask you to make an effort to improve the code following the standards stablished and make use of the expert advise of the people that is now watching the code and can help in the corrections.

Then there is the issue of auto. I have used the correct pointer at each place. Yet, I am being punished in the clang-tidy for knowing what the pointer is called and not just randomly assigning variables with auto.

I tend to agree with you on that comment. If the return type is clear and should not be modified and you know what you are doing taking that return type, I would not use auto. But I'm oldfashioned on my programming and I can understand the arguments on auto by @klenze and @YanzhaoW.

It's formatting. This has no impact on whether the code compiles. Why can I not format my code how I choose to? Why am I being forced to put a space after a //. Besides being pedantically idiotic, I, personally, don't like it. I thought we could be ourselves in coding, as long as what our code compiles and shows functionality. I thought that was what makes 'R3BRoot beautiful'???

You write code once (or rewrite it a few times), but you and many others read it many times. Having standards on the format is really important and help to structure code and sometimes to find errors. It is part of all scientific software development that I'm aware. Formatting with clang-format is a one-line process in the todo-list before a pull request. It should not be so stressful to produce a complain.

If you choose not to merge my code, fine. However, you will not have any other merge requests again from anyone. And you will have shown that pedantic idiocracy is more important to you than functional code.

Code is usually accepted after improvements during the PR processing (thank you to everyone that is contributing to the corrections and improvements). It could take some time when we could not have it: that is the reason why we could open alternative branches, as we discussed in the last analysis meeting, under certain conditions. But again, writing this statement after your PR, before any other comment, does not sound very productive. Would you accept suggestions on how to modify the code to make it better or we should take it as it is (or nothing)?

[ajedele]

I am very open to feedback on the functionality of the code. I am also open to obvious coding flags that affect the functionality or compileability of the code.

I have made sure the code compiles and that I see the ToT and hit pattern with a pulser

[klenze]

@YanzhaoW:

I would say it's better to get an error (either compile or run time) if you mess something up. :D The worst case is when you mess something up, you don't get an error but rather an implicit conversion. I think the second case is the biggest reason.

Consider

A* f();
void f(B*);

void foo()
{
    auto* ptr=f(); // line X
    g(ptr);        // line Y
}

If A* is implicitly convertable to B*, the only difference between using auto* and B* in line X is if the implicit conversion will happen in line X or Y.

If they are not implicitly convertible, then you will get an error in either case. If the incorrect assumption was "f returns B*", then an error in line X will probably be more useful than one in line Y.

They are some cases that could be very toxic due to implicit conversion. For example, if Class2 has implementation inheritance from Class1 (which means the method signatures between two classes are same but the implementations are different). Then code runs fine but result is wrong.

First of, I would define implementation inheritance as using inheritance to reuse code without a subtype relationship being implied (for example if you want to avoid the penalty of dynamic method dispatch). In C++, public inheritance implies a subtype relationship, hence the implicit type conversions. If you want pure implementation inheritance, the standard way is to place the base class in a namespace impl. Then g would have a signature void g(impl::BaseOfA*);, which is obviously evil.

Morale of the story: declaring variables with auto is insufficient to avoid bad implicit conversions. Having a class publicly inheriting from another class in an interface namespace when subtyping is not implied is an error.

@hapol: Reading between the lines of @ajedele's comments, I am beginning to suspect that she may in fact not consider making github pull requests her great calling in life. In fact, for someone coming more from the detector side of things, using all of

• C++ (the most byzantine language on the planet)
• our software framework
• cmake
• git
• the github workflow

just because they feels that we should have an online analysis for a detector during the next beam time is already a non-trivial ask. Getting then told just to write your own clang-tidy configuration to handle the CI issues might not be fully appreciated as the opportunity to learn more about a powerful and beautiful software tool that it objectively is. :)

I have two practical suggestions:
First, we should have a shell script for making pull requests. That shell script should do the following:

• Run clang-format (with autofix), clang-tidy and tests.
• Check that the branch is rebased on the latest dev
• Check that the commits are squashed, and offer to do so if they are not
• (optionally) Create the PR

(A bit more handholding with git would probably be even better. Assume that the user has not created a branch or committed their changes. "It seems like you are on branch dev. Would you like me to create a feature branch?", "Here are your uncommitted changes. Use git add to select the ones you want to include". Of course, handling every corner case is beyond the scope of anything short of a LLM (and possibly even them).)

The other suggestion is that we should not always treat PRs as being the responsibility of the one making them. I read @ajedele's messages more as "this is my contribution, I do not have time to improve it" than "either merge my PR verbatim as-is or not at all".

Like all our code, PRs are published under GPL3. This means that anyone can create another PR based on this one, polish it, tidy up all the warnings, add unit tests etc. From my reading of @jose-luis-rs comment, he basically suggested that he would do something to that effect.

[YanzhaoW]

@klenze

declaring variables with auto is insufficient to avoid bad implicit conversions.

This statement is incorrect. Auto is sufficient to avoid any implicit conversions. Your example is not very suitable as well since the type of ptr actually doesn't matter. ptr is passed to function g one way or another and the program's behaviour is solely depending on the definition of g rather than the type of ptr.

The case I was mentioning is:

A* f();

A* res = f();
res->DoSomething();

Here replacing with auto is less error-prone.

If you want pure implementation inheritance, the standard way is to place the base class in a namespace impl. Then g would have a signature void g(impl::BaseOfA*);, which is obviously evil.

Sure, choose whatever suitable name for your class. But I can't see the relevance here to auto. But let's not talk about it here since there could be a lot.

The "verdict" is use auto as much as possible because it's simpler, more reliable and less error-prone. It's just better and we should use it whenever possible.

[klenze]

How about "Declaring variables auto will prevent unwanted implicit conversions in that declaration, but will not change if implicit conversions happen in other contexts, thus one should avoid having a class hierarchy where implicit conversions are invalid."

[ajedele]

Philipp and I looked at the clang-tidy changes and I modified what we thought was reasonable.

I am still open to functionality suggestions.

Best,
Andrea

[ajedele]

BTW, we used the clang-format in R3BRoot. I don't understand why it failed

[ajedele]

Could you please give me feedback or merge the PR?

[jose-luis-rs]

I was teaching, sorry for that!
I will accept it because according to your details everything was tested with pulsers.