rsmith added a comment. In D106577#2904078 <https://reviews.llvm.org/D106577#2904078>, @aaron.ballman wrote:
> In D106577#2902091 <https://reviews.llvm.org/D106577#2902091>, @jyknight > wrote: > >> In D106577#2901654 <https://reviews.llvm.org/D106577#2901654>, @rsmith wrote: >> >>> I think this patch as it stands would cause problems with libc >>> implementations that put their `#define` somewhere other than than >>> `stdc-predef.h` (eg, older versions of glibc that put it in `features.h` or >>> other standard libraries that put it in `yvals.h`) due to the macro >>> redefinition with a conflicting expansion. Such implementations are >>> non-conforming but certainly exist. Likewise it could break the build for >>> the same reason if we start implicitly including `stdc-predef.h` at some >>> point. So I think that blindly predefining this macro will prove >>> problematic in at least some circumstances. >> >> I don't understand what problem you expect to see here. We already suppress >> -Wmacro-redefined [which this falls under] for system headers -- doesn't >> that suffice? > > I'd like to hear more about this as well, FWIW. I think we get all the > benefits you describe by defining the macro in the frontend and letting the > macro suppression work if there's a conflicting definition from a system > header. I'd expect we will break at least things like libc++ tests that build with `-Wsystem-headers` in the case where libc defines the macro. But if the problem is limited to old libc and a rare use case, and can easily be worked around with a `-Wno` flag, that's probably fine. One benefit we don't get with this approach is providing the right value for the macro (without paying the cost of always including `stdc-predefs.h`). AFAICS, the only possible use for the value of the macro is to detect libc support, so having Clang pick a specific value seems wrong to me. In some ways I'd be more comfortable with this patch if we defined the macro to `1` and documented that we think WG14 was wrong to ask for a version number. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106577/new/ https://reviews.llvm.org/D106577 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
