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

[Tom Anderson via Phabricator via libcxx-commits]

thomasanderson 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) {}
----------------
EricWF wrote:
> 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.
Done.  A new visibility macro is required since we presumably want _LIBCPP_INLINE_VISIBILITY when providing the inline definitions and not otherwise.  I tested that it works without _LIBCPP_INLINE_VISIBILITY, but I would think that we still want it.


================
Comment at: src/include/refstring.h:69
 
-inline
+_LIBCPP_REFSTRING_INLINE_VISIBILITY
 __libcpp_refstring::__libcpp_refstring(const char* msg) {
----------------
EricWF wrote:
> 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?
I understand why this is necessary now.  When the inline keyword is used, the compiler only generates code for that function if that function is actually used.  Since I removed the definitions of runtime_error etc in stdexcept.cpp, nothing actually uses __libcpp_refstring so the code doesn't get generated.


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

https://reviews.llvm.org/D58203