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

[Ian Anderson via Phabricator via libcxx-commits]

iana marked 2 inline comments as done.
iana 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:
> > 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.
> Err, it would be in the unwind PCM since unwind.h includes it, it just has the potential to get copied into other PCMs. @Bigcheese suggested we make it `private textual` to prevent that, let me give it a try. I still feel a little icky about making the module behavior differ from textual behavior (modules will emit an error if the unwind_ headers are directly included but textual won't), but it's not so bad.
> 
> I guess it's open to interpretation, but I see textual headers as things like assert.h where it's expected that you can include it multiple times with different values of `NDEBUG` in your code, or for X Macro generator headers. The unwind_ headers don't really fit either of those descriptions which is why I feel like they're a bad fit for textual, especially when it's a relatively small change to make them standalone.
> 
> That said, I can be out voted, what's really important to me is these belong to a module. I'll give `private textual` a try and see if it works out.
It looks like `private textual` works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144323