[PATCH] D129533: [ADT] Pass string_view via copy

[ppenguin via Phabricator via llvm-commits]

prehistoric-penguin added inline comments.


================
Comment at: llvm/include/llvm/ADT/Twine.h:303
     /// Construct from a StringRef.
-    /*implicit*/ Twine(const StringRef &Str) : LHSKind(PtrAndLengthKind) {
+    /*implicit*/ Twine(StringRef Str) : LHSKind(PtrAndLengthKind) {
       LHS.ptrAndLength.ptr = Str.data();
----------------
RKSimon wrote:
> prehistoric-penguin wrote:
> > RKSimon wrote:
> > > This probably should be done seperately and part of a larger refactor away from const StringRef & args IF we can confirm that its beneficial - it will probably need an update in the coding guidelines as well.
> > Thanks! Could you please elaborate more on `larger refactor`? I am interested in this.
> We have hundreds of uses of "const StringRef &" as function arguments - and the trade off will be to determine whether we get improvement in performance vs the additional cost of having to define StringRef in more headers.
> 
> <string> is already one of the most costly headers to include in llvm and making that worse (via including StringRef.h) doesn't sounds like a good idea: https://commondatastorage.googleapis.com/chromium-browser-clang/llvm-include-analysis.html
Thanks for the comment, I'm going to do some experiments and update the results of my research later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129533/new/

https://reviews.llvm.org/D129533