Details
Diff Detail
Event Timeline
source/Core/CxaDemangle.inc | ||
---|---|---|
1–17 ↗ | (On Diff #26538) | 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. |
source/Core/Mangled.cpp | ||
16–20 | 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. | |
50–57 | This function is going to get a "Not all control paths return a value" warning. Can you fix that? |
source/Core/CxaDemangle.inc | ||
---|---|---|
1–17 ↗ | (On Diff #26538) | 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. |
4849 ↗ | (On Diff #26538) | 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. |
source/Core/Mangled.cpp | ||
16–20 | 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. |
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 :)
Yeah, the the entire thing didn't compile on Windows before. Virgile Bello
tried to add these in r205769 but was reverted by emaste in r205776 since
it broke FreeBSD. I put these here because I think it's nicer to have
minimal changes to the cxa_demangler copy.
Well the thing is that in VS 2015 some of those features will start
working, like constexpr. It would be nice to have those C++ features start
working automatically once the LLVM macros are changed to reflect the new
compiler versions.
Sounds good to me. It wasn't just FreeBSD broken originally but probably anyone using clang (or any non-MSVC perhaps).
I think I was just the first to encounter the build failure.
source/Core/Mangled.cpp | ||
---|---|---|
64 | I'm personally not a fan of the implicit bool cast (here and below); do we already use this approach elsewhere? |
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.