[PATCH] D106186: Avoid keeping internal string_views in Twine.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/Twine.h:144
       const std::string *stdString;
-      const StringRef *stringRef;
-#if __cplusplus > 201402L
-      const std::string_view *stdStringView;
-#endif
+      StringRef stringRef;
       const SmallVectorImpl<char> *smallString;
----------------
I think from a standards-conformance perspective this may need to be decomposed into a raw struct of const char*+length - otherwise, since StringRef isn't trivially constructible (guess that's why you had to add the Child ctor?) it won't be validly constructed and deconstructed as required by C++ for non-trivial types like this.

At least that's my vague/general understanding. There could be some nuance that makes this valid.

(as a follow-up, it'd be good to collapse all the other cases that can be represented by StringRef into this case to reduce the complexity of this code, I think)


================
Comment at: llvm/include/llvm/ADT/Twine.h:155
       const uint64_t *uHex;
+      Child() : stringRef(){};
     };
----------------
If this does stay, the trailing semicolon should be removed 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106186