Closed TobiasStockmanns closed 4 months ago
CHANGELOG.md (18)
Near line 13: Unpaired symbol: ‘]’ seems to be missing Context: ...# Breaking Changes * CMake * Dropped [our custom `FindROOT.cmake`](https://git... --- Near line 53: ‘was in conflict with’ might be wordy. Consider a shorter alternative. Context: ...pped build switch `BUILD_UNITTESTS`, it was in conflict with the CMake standard switch [`BUILD_T... --- Near line 54: Unpaired symbol: ‘]’ seems to be missing Context: ...lict with the CMake standard switch [`BUILD_TESTING` from the CTest module](h... --- Near line 146: Consider inserting a comma after an introductory phrase for better readability. Context: ...es * Allow running without output sink. In this case even persistent branches would not be s... --- Near line 154: A comma may be missing after the conjunctive/linking adverb ‘Also’. Context: ... the Base into the Online library. * Also the MBS/LMD related source classes were... --- Near line 156: With the singular noun ‘one’, use the verb “is”. Context: ...ne library. * If one of these classes are used in your code, you need to link... --- Near line 174: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. Context: ... being set, rewrite it accordingly. * If your code has specific C++ needs, consi... --- Near line 175: Possible missing comma found. Context: ... CMake's `target_compile_features()`. * Dropped `CHECK_EXTERNAL_PACKAGE_INSTALL_DIR` ... --- Near line 177: This word is normally spelled as one. Context: ...`USE_PATH_INFO` CMake option. It was an anti-pattern. Don't use it! * Dropped `ROOT_GENERATE... --- Near line 193: The word “opt-in” is a noun. The verb is spelled with a space. Context: ...on Boost.Serialization. The user must opt-in to the previously baseline Boost serial... --- Near line 193: You used an adverb (‘previously’) instead of an adjective, or a noun (‘baseline’) instead of another adjective. Context: ...ialization. The user must opt-in to the previously baseline Boost serialization support by includ... --- Near line 206: The conjunction “so that” does not have a comma in front. Context: ...y require some API, please file an issue, so that we can see how to handle this. * Depre... --- Near line 218: Loose punctuation mark. Context: ...tion`. * `FairRadMapManager::Instance`, `FairRadLenManager::Instance` * `Fair... --- Near line 222: Loose punctuation mark. Context: ...thing. * `FairTask::*InputPersistance`: There's no getter for `fInputPersis... --- Near line 229: Loose punctuation mark. Context: ...ConstructASCIIGeometry(T*, TString, U*)`, use `FairModule::ConstructASCIIGeom... --- Near line 255: Possible missing comma found. Context: ...se` style naming. For those using the targets this change will not be visible. * Test... --- Near line 256: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause. Context: ...ing Geant3 have been disabled by default, because those tests have a probability > 0 fo... --- Near line 258: The word ‘anyways’ is informal American English. Did you mean “anyway”? Context: ... for failing. If you want to run them anyways, pass `-DENABLE_GEANT3_TESTING=ON` to...
fairroot/base/event/FairTimeStamp.h (1)
`77-77`: Change in `operator<` from pointer to reference type is correctly implemented. This change simplifies the comparison logic and aligns with C++ best practices by avoiding pointer arithmetic and potential null pointer dereferencing issues.
@TobiasStockmanns,
I don't understand why the operator is needed at all. I is neither used in FairRoot nor PandaRoot which was checked by Radek and me independently. Radek simply removed the function, I add the function with =delete
. In both cases we did not see any compilation errors. To cross-check I add a test which uses the operator< and this file shows the following error message during compilation
/opt/fairroot/source/v19.0/tests/base/event/test_FairTimeStamp.cxx:20:9: error: overload resolution selected deleted operator '<'
if (a<b) { a=2.;}
~^~
/opt/fairroot/source/v19.0/fairroot/base/event/FairTimeStamp.h:63:11: note: candidate function has been explicitly deleted
bool operator<(const FairTimeStamp& rValue) = delete;
^
/opt/fairsoft/jan24/include/root/TString.h:778:15: note: candidate function not viable: no known conversion from 'FairTimeStamp' to 'const TString' for 1st argument
inline Bool_t operator<(const TString &s1, const TString &s2)
^
/opt/fairsoft/jan24/include/root/TString.h:794:15: note: candidate function not viable: no known conversion from 'FairTimeStamp' to 'const TString' for 1st argument
inline Bool_t operator<(const TString &s1, const char *s2)
^
/opt/fairsoft/jan24/include/root/TString.h:812:15: note: candidate function not viable: no known conversion from 'FairTimeStamp' to 'const char *' for 1st argument
inline Bool_t operator<(const char *s1, const TString &s2)
^
/opt/fairroot/source/v19.0/fairroot/base/event/FairTimeStamp.h:64:11: note: candidate function not viable: no known conversion from 'FairTimeStamp' to 'const FairTimeStamp *' for 1st argument; take the address of the argument with &
bool operator<(const FairTimeStamp* rValue) = delete;
^
I don't see the need for the overloaded function and would propose to simply delete it or at least set it as deprecated.
The original idea why I defined bool operator<(const FairTimeStamp* rValue)
was to be able to sort a container of FairTimeStamp*
with this operator. Unfortunately this does not work because the sort will always use the memory location and not the FairTimeStamp::operator<
. So this implementation does not make any sense.
Nevertheless I think it is good to have an bool operator<(const FairTimeStamp& rValue)
defined, because for time based simulations and reconstructions a sorting by the time stamp is mandatory and this should be supported by the code.
In addition the virtual
should be removed.
@coderabbitai construct short CHANGELOG message for this PR. @TobiasStockmanns , can you squash into one commit?
[!TIP] For best results, initiate chat on the files or code changes.
@karabowi, here's a short CHANGELOG message for PR #1526:
- Updated `FairTimeStamp::operator<` to use references instead of pointers, aligning with standard C++ practices and enhancing safety. This change supports accurate timestamp-based sorting in simulations and reconstructions.
Let me know if you need any further modifications or additional information!
Please rebase and squash things.
Done.
The behaviour of the FairTimeStamp::operator< does no follow standard behaviour by dealing with the FairTimeStamp* instead of a FairTimeStamp&. This is fixed in this merge request. Should fix issue #1519.
Describe your proposal.
Mention any issue this PR resolves or is related to.
Checklist:
Summary by CodeRabbit
FairTimeStamp
class for enhanced reliability and performance.CHANGELOG.md
file.