[core] Pass TString by value to TNamed ctor

[ChristianTackeGSI]

This Pull request:

Passing by const-reference can mean two constructions (once on the caller, once in the ctor using the TString copy ctor).

Instead passing by value and using move semantics in the ctor can reduce this to one construction on the caller and one move in the ctor.

Checklist:

• [X] tested changes locally
• [ ] updated the docs (if necessary)

fixes: #15434

[github-actions[bot]]

Test Results

    11 files      11 suites   2d 16h 17m 3s :stopwatch:  2 645 tests  2 645 :white_check_mark: 0 :zzz: 0 :x: 27 455 runs  27 455 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 2bafc6ee.

:recycle: This comment has been updated with latest results.

[ChristianTackeGSI]

@guitargeek should I rebase or something?

[guitargeek]

I was just closing and opening to refresh the test results. I'm baffled that there are failures in what seems like a trivial PR!

[guitargeek]

Maybe we uncovered a problem with the TString move constructor? Any intuition, @pcanal?

[ChristianTackeGSI]

Rebased, so that the tests run against the latest version. Maybe it''s getting better?

[pcanal]

Maybe we uncovered a problem with the TString move constructor?

It's implementation is odd looking :(, so yeah I think this is the issue ... let me do a quick checks

[pcanal]

It's implementation is odd looking :(, so yeah I think this is the issue ... let me do a quick checks

and it actually seems to be working ... so now I'll try to reproduce the failure.

[pcanal]

I can reproduce the failure locally.

[ChristianTackeGSI]

I can add those comments to my PR. I am wondering, whether TNamed is the only affected class?

If not, then this warning should be put in some central place instead of scattered around here?

If I get this correctly, it could also affect user classes? I have opened a topic on the forum about this to discuss this outside this PR.

[pcanal]

I can add those comments to my PR. I am wondering, whether TNamed is the only affected class?

That is a good question.

If not, then this warning should be put in some central place instead of scattered around here?

My guess it that it would likely affect at least all classes that derives directly from TObject.

[pcanal]

So this problem has been known for 5 years :( https://github.com/root-project/root/pull/4320 but we manage to indeed lose track of it. That PR used the following more concise pattern:

TView() {} // NOLINT: not allowed to use = default because of TObject::kIsOnHeap detection, see ROOT-10300

where both the NOLINT is indeed useful to avoid spurious tool recommendation and the wording was clear but should probably be updated (ROOT-10300 is a ticket number in the JIRA instance which is now read-only)

It would be good to add some wording in the TObject documentation.

[ChristianTackeGSI]

So please suggest a concise wording for this PR and I will change it. I'd probably go for

  TNamed() {}   // your-wording

then.

[pcanal]

For the TObject documentation I would add something along the line of:

Classes derived from `TObject` can not use the `= default` syntax for their constructor as some compilers implement optimizations that prevents the `TObject::kIsOnHeap` detection mechanism from working properly.

[ChristianTackeGSI]

For the TObject documentation I would add something along the line of:

Classes derived from `TObject` can not use the `= default` syntax for their constructor as some compilers implement optimizations that prevents the `TObject::kIsOnHeap` detection mechanism from working properly.

Can we move this into another PR? (I would suggest you opening it), because it's still not clear whether the main target of the current PR is actually desired or not.

And TBH, I don't have the resources to perform a proper benchmark, nor do I have good other arguments why the proposed style is better (for example more readable, more maintainable, etc).

[pcanal]

An improvement on the TObject constructor doc is proposed at https://github.com/root-project/root/pull/16218