[PATCH] D103935: Add Twine support for std::string_view.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D103935#2883661 <https://reviews.llvm.org/D103935#2883661>, @MaskRay wrote:

> llvm/include has such `__cplusplus` dispatches in a few other places, e.g. `llvm/include/llvm/ADT/StringRef.h` L82 `strLen`. So those (-std=c++14 libLLVM.so + -std=c++17 application using llvm/include headers) users are already relying on benign ODR violation.
>
> LLVM has `-fvisibility-inlines-hidden`, and an additional -Bsymbolic-functions for safeguard, after linking into libLLVM.so, the ODR violation should be benign.
>
> Ensuring that struct/enum sizes are the same for different -std= modes is probably a sufficient fix to regular users (and a sufficiently good workaround for libLLVM.so with mixed -std= users).
>
> ---
>
> This thread does raise the question about what degree of support we want to provide for -DLLVM_LINK_LLVM_DYLIB=on users. Before previously shared object users were explicitly not supported. Now it may change as many Linux distributions have switched to -DLLVM_LINK_LLVM_DYLIB=on for their llvm/clang packages for size. With shared objects, we have to care about all sorts of compiler options which can cause non-benign ODR issues.

I think this will be an issue for static or dynamic linking, due to inlining. Importantly - if LLVM was built withoun string_view support but a caller was built with it and that caller tries to pass a string_view into some LLVM API then even if the layout and enum elements were consistent, we'd still hit UB/assert/etc because the Twine passed in would have that unexpected enum value which wouldn't be covered by the Twine code inside LLVM.

Having the implementation be the same (by using some form of char*+length representation which would always be supported), having an extra ctor that takes string_view but lowers to that same representation should be safe (yes, a benign ODR violation - the debug info will be a bit weird/missing that extra function/ctor - but codewise, I can't think of any way it wouldn't work).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103935