Fix a number of clang-tidy detected defects#22416
Conversation
Test Results 21 files 21 suites 3d 11h 33m 59s ⏱️ For more details on these failures, see this check. Results for commit e299cd1. ♻️ This comment has been updated with latest results. |
dpiparo
left a comment
dpiparo
left a comment
There was a problem hiding this comment.
Thanks for these changes! Would it be useful to expand this kind of campaigns to other portions of the codebase, e.g. roofit or RDF, or explore other checks?
| { | ||
| return 0 == name.compare(0, 17, "std::__pair_base<") || 0 == name.compare(0, 12, "__pair_base<"); | ||
| } | ||
| inline std::string GetUniquePtrType(std::string_view name) |
There was a problem hiding this comment.
TClassEdit is a public interface and thus this 'could' be used outside of ROOT. Should we deprecated it instead of removing it?
There was a problem hiding this comment.
Note that the function as is returns a temporary, so it may crash if actually used. No one complained so far.
I can still deprecate it but I'd have a preference for removing. Let me know if you'd prefer a slower removal.
|
Thanks for this, it's great!
Yes. Maybe even sth like: |
Maybe. I think the next step would be a regular clang-tidy check for RNTuple as part of the CI. Looking into it. |
jblomer
force-pushed
the
tidy
branch
2 times, most recently
from
b6666a1 to
45e70d6
Compare
silverweed
left a comment
silverweed
left a comment
There was a problem hiding this comment.
Thanks! I have just a few comments
| RPage(const RPage &) = delete; | ||
| RPage &operator=(const RPage &) = delete; | ||
| RPage(RPage &&other) | ||
| RPage(RPage &&other) noexcept |
There was a problem hiding this comment.
Any reason to make this particular class's move ctor noexcept?
EDIT: I see there are more later on, see the other comment about it
| mutable std::atomic<const std::type_info *> fTypeInfo = nullptr; | ||
|
|
||
| RValue(RFieldBase *field, std::shared_ptr<void> objPtr) : fField(field), fObjPtr(objPtr) {} | ||
| RValue(RFieldBase *field, std::shared_ptr<void> objPtr) : fField(field), fObjPtr(std::move(objPtr)) {} |
There was a problem hiding this comment.
Are these moves actually making a difference in the generated assembly?
There was a problem hiding this comment.
The difference is between the std::shared_ptr copy constructor and move constructor, i.e. whether or not the atomic counter is incremented and decremented.
There was a problem hiding this comment.
Isn't objPtr's counter incremented/decremented anyway due to RValue receiving it by copy?
| } | ||
| RValue(RValue &&other) : fField(other.fField), fObjPtr(std::move(other.fObjPtr)) {} | ||
| RValue &operator=(RValue &&other) | ||
| RValue(RValue &&other) noexcept : fField(other.fField), fObjPtr(std::move(other.fObjPtr)) {} |
There was a problem hiding this comment.
Do we in general want to adopt this convention? I vaguely know this might have some performance implications in certain cases, but is this something worth committing to? (I'm personally not a fan of noexcept but we can discuss if it can have a concrete impact)
There was a problem hiding this comment.
My motivation was to ensure that STL containers move elements, however I see that in many cases the copy constructor is deleted anyway. Probably only relevant still for RVec and RActiveEntryToken. We can also just drop this aspect (exclude the checks) and come back to it later if needed.
A copy constructor of an exception should not throw. Otherwise, during `throw RException(...)`, a second exception may be thrown that would immediately terminate the program. The fError member may throw due to the memory allocation in its string and vector members.
With commit c5ee8b5 (enable unique_ptr support for nested STL collections), the method TClassEdit::GetUniquePtrType() became obsolete.
6 participants
A test run of clang-tidy on the RNTuple code with the following checks