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

aidengrossman added a comment. > I believe that /Ze has more in common with `-fms-compatibility` than > `-fms-extensions`, but I could be wrong. Also, they may just be completely > different things at this point. `/permissive` is closer in spirit to > `-fms-compatibility`. Better

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

rnk added a comment. > Also assuming that /Ze is similar to our -fms-extensions which I believe is > the case, but not completely sure I believe that /Ze has more in common with `-fms-compatibility` than `-fms-extensions`, but I could be wrong. Also, they may just be completely different

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

aidengrossman added a comment. In 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 >

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

rnk added a comment. 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 say that `/Za`/`/Ze` control `_MSC_EXTENSION`, and as I understand it,

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

aidengrossman updated this revision to Diff 550529. aidengrossman added a comment. Hoist _MSC_EXTENSIONS macro logic even further and add release note on chages. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157334/new/

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

aaron.ballman added a comment. I'm comfortable with the changes but we should add a release note so users know about the change in behavior. I also wonder if we should take this opportunity to add some user-facing documentation around how the target triple, `-fms-compatibility`, and

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

hans added a comment. In D157334#4573902 , @aaron.ballman wrote: > I'm curious to hear what others think about this (especially @rnk and @hans). Seems reasonable to me. (Looks like the define was originally added here:

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

aaron.ballman added a comment. In D157334#4574197 , @aidengrossman wrote: > It's interesting that GCC doesn't set the `_MSC_EXTENSIONS` macro with > `-fms-extensions`. It should if it wants to match the behavior of MSVC. FWIW, I filed

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

aidengrossman added a comment. It's something set on by default in MSVC (https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170). It's interesting that GCC doesn't set the `_MSC_EXTENSIONS` macro with `-fms-extensions`. It should if it wants to match the behavior of

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

mstorsjo added a comment. I think this makes sense in principle - however it does diverge from GCC. GCC also supports `-fms-extensions` (but I'm not sure if the set of extensions that are enabled by the option is an exact match between the two cases), but GCC doesn't expose any extra define in

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

aidengrossman added a comment. Since `-fms-extensions` can be enabled on Linux as well, we should probably hoist this further since this patch only accounts for the windows case as I just hoist the conditional to be in `addWindowsDefines`. I'll work on getting another patch version up in a

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

aaron.ballman added a comment. I'm curious to hear what others think about this (especially @rnk and @hans). I think this is the correct approach -- `-fms-extensions` is separate from the Windows target; for example, users can enable `__declspec` attributes this way:

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

aidengrossman updated this revision to Diff 548473. aidengrossman added a comment. Reformat using arc as manually uploading patch messes up formatting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157334/new/ https://reviews.llvm.org/D157334

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

aidengrossman added reviewers: aaron.ballman, hans, mstorsjo, rnk, glandium. aidengrossman added a comment. This should fix the issues seen in https://reviews.llvm.org/D150646 regarding `-fms-extensions` being set on MinGW and the builtins being defined but no `_MSC_EXTENSIONS` macro being set

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

aidengrossman created this revision. Herald added subscribers: pengfei, mstorsjo. Herald added a project: All. aidengrossman requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This patch makes sure _MSC_EXTENSIONS is defined if