[libcxx-commits] [PATCH] D144323: [libunwind][Modules] Add unwind_arm_ehabi.h and unwind_itanium.h to the unwind module)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libunwind/include/unwind_arm_ehabi.h:16-18
+#include <__libunwind_config.h>
+
+#ifdef _LIBUNWIND_ARM_EHABI
----------------
iana wrote:
> ldionne wrote:
> > Why is this needed? We only include those after checking `#if defined(_LIBUNWIND_ARM_EHABI)`.
> The module will build all of its headers, so without these guards you'll get a lot of redeclaration errors between the two unwind_ headers.
> 
> It might maybe work to put them in independent modules and mark them as conflicting. I don't really think that's the right thing to do since these two headers are just implementation details for unwind.h.
> 
> We could make them textual, but then they don't get put in the pcm which isn't what we want.
> 
> We could make them excluded and then one or the other should be pulled into unwind when it builds, but that's a fragile mechanism since some other module could unexpectedly include them. It's also not really what we're trying to do, they should be part of unwind and not really excluded.
> 
> So I think the right thing to do is make each of these headers standalone and able to be built in the same module.
The problem is that this is a general pattern we have in a lot of places. Today, we use headers as a mechanism to (quite textually) separate code into a different physical location. These headers are implementation details and they are never intended to be included directly by the user. I'm not a modules expert, but it looks to me like those are effectively textual headers that are meant to be copy-pasted into `unwind.h` depending on the API we want to provide. If so, we should make them textual even if that has a negative impact on the generated PCM, since that is the proper semantics of those headers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144323