[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

Thu Aug 17 15:03:13 PDT 2023

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