[PATCH] D150966: [clang] Don't define predefined macros multiple times

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:241-242
   Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
-  Builder.defineMacro("__ARM_FEATURE_ATOMICS", "1");
-  Builder.defineMacro("__ARM_FEATURE_CRC32", "1");
 }
----------------
Hmm, is this correct?

`__ARM_FEATURE_ATOMICS` is defined in one other place, but it's conditionally defined: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L475

and `__ARM_FEATURE_CRC32` is defined in two places, both conditional: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/AArch64.cpp#L422 and https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Basic/Targets/ARM.cpp#L747




================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:343-347
-  // Target properties.
-  if (!getTriple().isOSWindows() && getTriple().isArch64Bit()) {
-    Builder.defineMacro("_LP64");
-    Builder.defineMacro("__LP64__");
-  }
----------------
I think this is correct -- the other definition has a different predicate (https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Frontend/InitPreprocessor.cpp#L949) but I think it ends up being define in exactly the same scenarios.


================
Comment at: clang/lib/Basic/Targets/VE.cpp:30-32
-  Builder.defineMacro("unix", "1");
-  Builder.defineMacro("__unix__", "1");
-  Builder.defineMacro("__linux__", "1");
----------------
Shouldn't this be calling `DefineStd(Builder, "unix", Opts);` like all the others?


================
Comment at: clang/lib/Basic/Targets/VE.cpp:35
   Builder.defineMacro("__ve__", "1");
-  Builder.defineMacro("__STDC_HOSTED__", "1");
-  Builder.defineMacro("__STDC__", "1");
----------------
This is technically a breaking change but I think it's a good breaking change. The VE target seems to imply that there's never a freestanding mode unlike the general utility: https://github.com/llvm/llvm-project/blob/11926e6149d2a68ecb0652b248efe6890c163846/clang/lib/Frontend/InitPreprocessor.cpp#L431


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150966/new/

https://reviews.llvm.org/D150966