[libcxx-commits] [PATCH] D58203: [libc++] Inline stdexcept constructors, destructors, and assignment operators when using MSVC ABI

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: include/stdexcept:83
     explicit logic_error(const string&);
+#if defined(_LIBCPP_ABI_INLINE_STDEXCEPT_DEFINITIONS)
+    _LIBCPP_INLINE_VISIBILITY explicit logic_error(const char* __msg) : __imp_(__msg) {}
----------------
We should declare the same set of symbols regardless of `_LIBCPP_ABI_INLINE_STDEXCEPT_DEFINITIONS`.

And when we want inline definitions, instead of defining it inside the class, let's put an inline definition just below the class and guard that.


================
Comment at: src/include/refstring.h:69
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const char* msg) {
----------------
thomasanderson wrote:
> EricWF wrote:
> > These changes seem wrong. And I don't understand why the changes are needed in relation to MSVC.
> This is now needed eg. for the inline definition of logic_error::logic_error(), which must now construct a __libcpp_refstring.
> 
> If I leave "inline" in, I get link errors:
>     lld-link: error: undefined symbol: "public: __cdecl std::__1::__libcpp_refstring::__libcpp_refstring(char const *)" (??0__libcpp_refstring at __1@std@@QEAA at PEBD@Z)
>     >>> referenced by obj/buildtools/third_party/libc++/libc++/string.obj
> 
> include/refstring.h gets included in stdexcept.cpp.  Actually, that's the only place it gets included, so maybe we could get rid of the inline altogether since we can't have multiple definitions?
I don't understand that error at all.

The constructor it's saying is undefined is marked `inline`. If it is ODR-used in a translation unit, then MSVC needs to emit a definition for it in that TU.

Am I missing something?


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

https://reviews.llvm.org/D58203