[PATCH] D129533: [ADT] Pass string_view via copy
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 14 03:45:19 PDT 2022
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
More information about the llvm-commits
mailing list