aidengrossman added a comment. In D157334#4596328 <https://reviews.llvm.org/D157334#4596328>, @rnk wrote:
> I don't think this is the right thing to do, I think I recall saying as much > on some other review. The MSVC docs > <https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170> > say that `/Za`/`/Ze` control `_MSC_EXTENSION`, and as I understand it, `/Za` > is more like our `-fms-compatibility` flag, so it makes sense to control this > macro with `-fms-compatibility`. From what I can understand from this page <https://learn.microsoft.com/en-us/cpp/build/reference/za-ze-disable-language-extensions?view=msvc-170>, `/Ze` enables the MSVC extensions (which is set by default) and `/Za` disables the MSVC extensions. From what I can gather reading the Clang options docs <https://clang.llvm.org/docs/ClangCommandLineReference.html>, `-fms-compatibility` is intended to be a superset of `-fms-extensions`. Assuming those two things, I think it's logical to set `_MSC_EXTENSIONS` with `-fms-extensions` rather than `-fms-compatibility`. (Also assuming that `/Ze` is similar to our `-fms-extensions` which I believe is the case, but not completely sure). > Even though the name of the macro and the flag correspond, they aren't > covering the same thing. > > Let's try to focus on the use case: We need a feature flag or macro to > communicate that `-fms-extensions` is enabled on mingw. `_MSC_VER` is out > because we're doing mingw. Is there something else we could check for like > `__has_declspec_attribute` or `__has_builtin`? `__has_builtin` works (no idea why I didn't think of this, thank you very much for the suggestion) for that case. Like you mentioned in the https://reviews.llvm.org/D150646, exposing the builtins with `-fms-extensions` doesn't make sense, and that should probably be fixed at some point. That should probably be left to another patch though, along with other issues related to when builtins get exposed (like the previously mentioned aux triple issue). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157334/new/ https://reviews.llvm.org/D157334 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
