root icon indicating copy to clipboard operation
root copied to clipboard

Consider passing TString by value to TNamed ctor

Open ChristianTackeGSI opened this issue 3 months ago • 6 comments

Explain what you would like to see improved and how.

The TNamed ctor currently takes TString by const reference:

TNamed::TNamed(const TString& name, const TString& title)

Reading https://www.cppstories.com/2018/08/init-string-member/ it seems, that it would be a good idea to instead pass it by value and then use move in the ctor?

-   TNamed(const TString &name, const TString &title) : fName(name), fTitle(title) { }
+   TNamed(TString name, TString title) : fName(std::move(name)), fTitle(std::move(title)) { }

ROOT version

master

Installation method

none, just looking at the source code.

Operating system

any

Additional context

We're currently looking at improving the ctors of our classes and noticed the double construction of TString, if we don't pass as const char*.

ChristianTackeGSI avatar May 07 '24 10:05 ChristianTackeGSI

Nice idea! Can you open a PR for that? This is a very common case indeed, it's nice to optimize it.

By the way, there are clang-tidy checks to do this automatically.

guitargeek avatar May 07 '24 14:05 guitargeek

Nice idea! Can you open a PR for that? This is a very common case indeed, it's nice to optimize it.

Yes, I could. But not before next week (I am quite busy this week).

By the way, there are clang-tidy checks to do this automatically.

Oh, I didn't know. Which ones do you mean?

ChristianTackeGSI avatar May 07 '24 14:05 ChristianTackeGSI

Note that to be effective this change would also require the addition of a move constructor in TString.

pcanal avatar May 07 '24 22:05 pcanal

By the way, there are clang-tidy checks to do this automatically.

Oh, I didn't know. Which ones do you mean?

This looks like one: https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html

dennisklein avatar May 08 '24 06:05 dennisklein

Note that to be effective this change would also require the addition of a move constructor in TString.

I think this exists already? https://root.cern.ch/doc/master/classTString.html#a97795e61556ec0ced81e15cdfa26a538

TString::TString(TString&& s)

ChristianTackeGSI avatar May 08 '24 10:05 ChristianTackeGSI

Indeed. I missed it :)

pcanal avatar May 08 '24 17:05 pcanal