Added testing boilerplate, basic tests and global formatting #51
Conversation
src/TRestRawSignal.cxx
Outdated
| if (fGraph != NULL) { | ||
| if (fGraph) { |
There was a problem hiding this comment.
imo abusing implicit conversion from a type T to bool is bad practice. This notation should only be used for actual bool variables.
There was a problem hiding this comment.
I am not sure if I agree with you here, for example in https://stackoverflow.com/questions/3825668/checking-for-null-pointer-in-c-c many people prefer this approach. I guess its a matter of preference but in my opinion this is more concise and thus better, I can't image how this could be a problem, even smart pointers support this notation.
There was a problem hiding this comment.
In terms of "correctness" if you will, this is perfectly fine of course. But if you consider programming from a standpoint of being an asynchronous discussion between multiple people reading the code, the concise approach is unclear to the reader that isn't perfectly aware of the full picture. There's no way to know that fGraph here is some pointer. It very well may be a flag that decides whether some graphs are created!
In any more strongly typed language you'd have to type this out fully as well. And for good reason.
|
Too much formatting will overwhelm the reviewing. Maybe you can create a single PR to just do the global formatting? |
Yes, maybe I got a bit carried away. But the changes to files others than |
…t so often in the future
|
Ok, pipeline is solved because of the fix introduced at Warning that PR should be merged before! |
I added the
testdirectory with basic tests. rest-for-physics/framework#163I also did some global formatting to the whole library without introducing any changes, unless I made some mistake.