[Lldb-commits] [PATCH] Allow both MSVC and Itanium mangling schemes.

[Zachary Turner]

I mean I see
+#define noexcept
+#define constexpr
+#define snprintf _snprintf
+#define alignas(X) __declspec(align(X))

but I dont' see corresponding -'s from anywhere else in the diff.  So it
looks like this is new code, not moved from somewhere else.  Unless I'm
just missing something.  But now that I think about it, it looks like it's
just because all this was previously compiled out on Windows, and now it's
being compiled on Windows.

Since we've inlined a copy of the code already anyway, personally I don't
see anything wrong with making small changes like this just to get it to
compile on all platforms.  I mean I wouldn't think it wise to change some
of the actual logic of the demangling, but just adding decorators on
functions to get it to compile would be ok I would think?  Others may
disagree though :)

On Tue, May 26, 2015 at 3:10 PM Chaoren Lin <chaorenl at google.com> wrote:

> ================
> Comment at: source/Core/CxaDemangle.inc:1-17
> @@ -1,14 +1,18 @@
>  //----------------------------------------------------------------------
>  // Inlined copy of:
>  // http://llvm.org/svn/llvm-project/libcxxabi/trunk/src/cxa_demangle.cpp
> -// revision 199944.
> +// revision 235830.
>  //
>  // Changes include:
> -// - remove the "__cxxabiv1" namespace
> -// - stripped GCC attributes()
> -// - removed extern "C" from the cxa_demangle function
> +// - Removed the "__cxxabiv1" namespace
> +// - Stripped GCC attributes()
> +// - Removed extern "C" from the cxa_demangle function
>  // - Changed the scope of the unnamed namespace to include cxa_demangle
>  //   function.
>  // - Added "#undef _LIBCPP_EXTERN_TEMPLATE" to avoid warning
> +// - Removed constexpr member initialization for MSVC
> +// - Implemented missing rebind, construct, destroy in malloc_alloc
> +//   for MSVC
> +// - Changed argument to alignas() to a literal for MSVC
>  //----------------------------------------------------------------------
>
> ----------------
> zturner wrote:
> > Unless we have a specific reason not to (which nobody has spoken up
> about yet), I would prefer for this to be a standalone .cpp file which
> compiles to an object file, instead of a .inc file.  I can't think of any
> reason not to do this, but +greg and ed maste in case there is something
> about their platforms which would necessitate it being an inline file.
> I think it's for the same reason that we're putting everything here in an
> anonymous namespace. To prevent collision with a system defined
> `cxa_demangle`. I guess we could also solve that problem by putting
> everything under lldb_private.
>
> ================
> Comment at: source/Core/CxaDemangle.inc:4849
> @@ -4816,1 +4848,3 @@
>      }
> +    template<class Other>
> +    struct rebind
> ----------------
> Do you know the rebind, construct, and destroy are required for allocators
> in the STL standard, or it's just a MSVC quirk? If it's the former I think
> we should try to upstream it to libcxxabi, and avoid changing it here.
>
> ================
> Comment at: source/Core/Mangled.cpp:16-20
> @@ -15,2 +15,7 @@
>  #include <Dbghelp.h>
> +#define LLDB_USE_BUILTIN_DEMANGLER
> +#define noexcept
> +#define constexpr
> +#define snprintf _snprintf
> +#define alignas(X) __declspec(align(X))
>  #elif defined (__FreeBSD__)
> ----------------
> zturner wrote:
> > Is there a reason why we didn't need these defines before but we need
> them now?
> >
> > In any case, llvm has LLVM_NOEXCEPT, LLVM_CONSTEXPR, and LLVM_ALIGNAS,
> which all just do the right thing depending on the platform.  Seems like we
> should use those instead.
> What do you mean by "before"? The builtin demangler wasn't enabled for
> MSVC. I'll try to upstream the LLVM_* change to libcxxabi, so we don't have
> to change them in our copy.
>
> http://reviews.llvm.org/D10040
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150526/9e80b2be/attachment.html>