compnerd added inline comments. ================ Comment at: lib/Headers/unwind.h:61 @@ +60,3 @@ +#define _UNWIND_ARM_EHABI 0 +#endif + ---------------- logan wrote: > compnerd wrote: > > logan wrote: > > > Since this is `unwind.h`, I feel that we can get a step further and use > > > `__ARM_EABI_UNWINDER__` to get more compatibility to GCC's unwind.h. > > > > > > Here's the change: > > > > > > ``` > > > #if defined(__arm__) && !defined(__USING_SJLJ_EXCEPTIONS__) && \ > > > !defined(__ARM_DWARF_EH__) > > > #define __ARM_EABI_UNWINDER__ 1 > > > #endif > > > ``` > > I dont know if we really need to imitate GCC's macros here. Am I mistaken > > in that they assume that `__ARM_EABI_UNWINDER__` has been set to 1 > > externally if targeting such an environment? I think that it is better to > > use the reserved namespace and intrude into libunwind's namespace as > > already done here. > > Am I mistaken in that they assume that `__ARM_EABI_UNWINDER__` has been set > > to 1 externally if targeting such an environment? > > Although this is an implementation detail, it was defined by `unwind.h` in > the implementation of GCC. > > Remark: `__ARM_EABI_UNWINDER__` is not a pre-defined macro in GCC and Clang > (can be checked with ` gcc -dM -E - < /dev/null`.) > > BTW, some applications or libraries need this macro to be defined after > including `<unwind.h>` (such as uclibc, boost, or libc++abi 3.0.) I > remembered that someone suggested to use `__ARM_EABI_UNWINDER__` instead of > `LIBCXXABI_ARM_EHABI` when I was fixing libc++abi several years ago. I chose > `LIBCXXABI_ARM_EHABI` simply because `__ARM_EABI_UNWINDER__` wasn't provided > by clang at that time. > > I am less concerned to namespace pollution, because this is already the de > facto implementation in GCC world and the macro names start with underscores > are reserved for compiler or standard libraries by convention. > > Since this is file a public header and will be used for a long time, I > personally believe that it will be better to use an existing name with the > same meaning instead of introducing a new name. In addition, this will make > it easier to port the application between gcc and clang. I just checked, libc++abi has no use of this macro, nor does boost 1.60. uclibc only defines `__ARM_EABI_UNWINDER__`, but does not use it. I also checked glibc and musl, and glibc like uclibc defines it while musl has no references to it. This is injecting itself into the compiler namespace and is misleading, so I think I would really rather prefer the current patch as is.
http://reviews.llvm.org/D15883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
