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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon 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();
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129533