Fix a number of clang-tidy detected defects by jblomer · Pull Request #22416 · root-project/root

jblomer · 2026-05-28T12:14:19Z

A test run of clang-tidy on the RNTuple code with the following checks

Checks: 'bugprone-*,-bugprone-assignment-in-if-condition,-bugprone-branch-clone,-bugprone-derived-method-shadowing-base-method,-bugprone-easily-swappable-parameters,-bugprone-implicit-widening-of-multiplication-result,-bugprone-incorrect-enable-if,-bugprone-macro-parentheses,-bugprone-multi-level-implicit-pointer-conversion,-bugprone-narrowing-conversions,-bugprone-not-null-terminated-result,-bugprone-random-generator-seed,-bugprone-reserved-identifier,-bugprone-signed-char-misuse,-bugprone-std-namespace-modification,-bugprone-switch-missing-default-case,-bugprone-throwing-static-initialization,-bugprone-too-small-loop-variable,-bugprone-unchecked-optional-access,-bugprone-unhandled-self-assignment,-bugprone-unused-return-value,-bugprone-virtual-near-miss,cppcoreguidelines-virtual-class-destructor,misc-*,-misc-non-private-member-variables-in-classes,-misc-include-cleaner,-misc-misplaced-const,-misc-multiple-inheritance,-misc-no-recursion,misc-non-copyable-objects,-misc-non-private-member-variables-in-classes,-misc-predictable-rand,-misc-unconventional-assign-operator,-misc-use-anonymous-namespace,performance-*,-performance-enum-size,-performance-inefficient-string-concatenation,-performance-no-int-to-ptr,-performance-noexcept-swap,readability-duplicate-include,readability-delete-null-pointer,readability-misleading-indentation,readability-redundant-control-flow,readability-redundant-declaration,readability-redundant-function-ptr-dereference,readability-reference-to-constructed-temporary'

github-actions · 2026-05-28T16:08:11Z

Test Results

20 files 20 suites 3d 6h 35m 22s ⏱️
3 865 tests 3 852 ✅ 0 💤 13 ❌
68 876 runs 68 745 ✅ 118 💤 13 ❌

For more details on these failures, see this check.

Results for commit 135b5ba.

♻️ This comment has been updated with latest results.

dpiparo

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?

pcanal · 2026-06-03T21:44:36Z

   {
      return 0 == name.compare(0, 17, "std::__pair_base<") || 0 == name.compare(0, 12, "__pair_base<");
   }
-   inline std::string GetUniquePtrType(std::string_view name)


TClassEdit is a public interface and thus this 'could' be used outside of ROOT. Should we deprecated it instead of removing it?

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.

pcanal

LGTM.

ferdymercury · 2026-06-04T09:46:47Z

Thanks for this, it's great!

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?

Yes. Maybe even sth like:
https://github.com/HorstBaerbel/action-clang-tidy
or
https://github.com/marketplace/actions/clang-tidy-review
https://github.com/marketplace/actions/c-c-linter

jblomer · 2026-06-05T12:02:38Z

Thanks for this, it's great!

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?

Yes. Maybe even sth like: https://github.com/HorstBaerbel/action-clang-tidy or https://github.com/marketplace/actions/clang-tidy-review https://github.com/marketplace/actions/c-c-linter

Maybe. I think the next step would be a regular clang-tidy check for RNTuple as part of the CI. Looking into it.

hahnjo

LGTM

hahnjo · 2026-06-08T13:36:42Z

-   ~RResult() = default;
-
   /// Used by R__FORWARD_RESULT in order to keep track of the stack trace in case of errors
   RResult &Forward(RError::RLocation &&sourceLocation)


Still taking xvalue reference, not sure if this should also take const ref

hahnjo · 2026-06-08T13:36:48Z

@@ -280,7 +303,7 @@ public:
   RResult &Forward(RError::RLocation &&sourceLocation)


Still taking xvalue reference, not sure if this should also take const ref

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.

…rs()

silverweed

Thanks! I have just a few comments

silverweed · 2026-06-15T15:29:19Z

+   {
+      // 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.


If we OOM, don't we want to terminate the program anyway?

silverweed · 2026-06-15T15:34:50Z

 std::string GetRenormalizedMetaTypeName(const std::string &metaNormalizedName)
 {
-   const auto canonicalTypePrefix = ROOT::Internal::GetCanonicalTypePrefix(metaNormalizedName);
+   auto canonicalTypePrefix = ROOT::Internal::GetCanonicalTypePrefix(metaNormalizedName);


How is this improving the performance?

silverweed · 2026-06-15T15:35:28Z

   RPage(const RPage &) = delete;
   RPage &operator=(const RPage &) = delete;
-   RPage(RPage &&other)
+   RPage(RPage &&other) noexcept


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

silverweed · 2026-06-15T15:37:18Z

   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)) {}


Are these moves actually making a difference in the generated assembly?

silverweed · 2026-06-15T15:38:54Z

   }
-   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)) {}


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)

jblomer self-assigned this May 28, 2026

jblomer requested review from dpiparo and pcanal as code owners May 28, 2026 12:14

jblomer force-pushed the tidy branch from 8b910fd to 0aaf399 Compare June 2, 2026 11:44

dpiparo approved these changes Jun 3, 2026

View reviewed changes

pcanal reviewed Jun 3, 2026

View reviewed changes

pcanal approved these changes Jun 3, 2026

View reviewed changes

hahnjo self-requested a review June 4, 2026 11:32

hahnjo reviewed Jun 4, 2026

View reviewed changes

Comment thread core/foundation/inc/ROOT/RError.hxx

Comment thread core/foundation/inc/ROOT/RError.hxx Outdated

Comment thread core/foundation/src/RError.cxx Outdated

jblomer force-pushed the tidy branch 2 times, most recently from b6666a1 to 45e70d6 Compare June 8, 2026 13:28

hahnjo approved these changes Jun 8, 2026

View reviewed changes

jblomer force-pushed the tidy branch from 45e70d6 to a8e52b4 Compare June 11, 2026 09:38

jblomer added the clean build Ask CI to do non-incremental build on PR label Jun 11, 2026

jblomer force-pushed the tidy branch from a8e52b4 to 135b5ba Compare June 15, 2026 15:19

jblomer requested review from hageboeck and silverweed as code owners June 15, 2026 15:19

jblomer added 10 commits June 15, 2026 17:20

[ntuple] fix argument comment

36c5de7

[io] fix casting through void

4bf169b

[RVec] avoid unnecessary capacity check in internal set_size

b29e48f

[ntuple] avoid throwing impossible exception

ddd2b46

[ntuple] don't throw in RNTupleModel::~RUpdater()

46611a6

[ntuple] safer use of RException

78cdebd

[ntuple] fix throwing exception in RNTupleDescriptor

cd9642d

[ntuple] remove wrong forward declaration of RFieldVisitor

8d068c3

[core] fix definition of kMaxULong

f98406e

jblomer added 26 commits June 15, 2026 17:20

[ntuple] minor improvement in optional assignments

6aa2a73

[meta] remove unused TClassEdit::GetUniquePtrType()

530941a

With commit c5ee8b5 (enable unique_ptr support for nested STL collections), the method TClassEdit::GetUniquePtrType() became obsolete.

[ntuple] minor readability improvement

fe593af

[ntuple] minor improvement in RPageSourceFile::CreateFromAnchor()

bf5564b

[ntuple] remove unused variable in RFieldBase::Create()

e4e57ba

[ntuple] fix use-after-move in RNTupleDescriptorBuilder::AddColumn()

2b8d419

[NFC][ntuple] suppress clang-tidy false positive

8076e16

[ntuple] fix widening method visibility change

9f5ed0a

[ntuple] remove unusued usings

b50926d

[ntuple] private linkage of stream operator in the merger

89e9917

[ntuple] replace std::endl by \n

1ecb4e9

[ntuple] minor performance improvement in RPageSourceFile::LoadCluste…

d01a3d3

…rs()

[core] pass RError::RLocation by const ref

9d1c3ce

[ntuple] use member move in RValue move ctor

0c45520

[ntuple] minor perf improvement in type name normalization

a01c9ac

[ntuple] make moving RPage(Ref) noexcept

56439bb

[ntuple] avoid some unnecessary value copies

016b13a

[core] remove unnecessary declaration in RResult

dc6e376

[ntuple] mark some move constructors noexcept

287a2bb

[core] make moving TTaskGroup noexcept

6063ed3

[RVec] improve noexcept specification

5a325cc

[ntuple] fix virtual destructor visibility

f10113c

[ntuple] fix up RNTupleDescriptorBuilder::SetNTuple() signature

6c1b013

[ntuple] remove duplicate includes

6e4df83

[core] minor improvement in TIterator

fa3aebc

[NFC][ntuple] suppress cling-tidy warning for RKeyBlob

135b5ba

silverweed reviewed Jun 15, 2026

View reviewed changes

		@@ -280,7 +303,7 @@ public:
		RResult &Forward(RError::RLocation &&sourceLocation)

Conversation

jblomer commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

dpiparo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcanal left a comment

Choose a reason for hiding this comment

Uh oh!

ferdymercury commented Jun 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jblomer commented Jun 5, 2026

Uh oh!

hahnjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverweed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverweed Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jblomer commented May 28, 2026 •

edited

Loading

github-actions Bot commented May 28, 2026 •

edited

Loading

silverweed Jun 15, 2026 •

edited

Loading