[PATCH] D85324: [z/OS] Add z/OS Target and define macros

[Abhina Sreeskantharajan via Phabricator via cfe-commits]

abhina.sreeskantharajan marked 2 inline comments as done.
abhina.sreeskantharajan added inline comments.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:732
+    Builder.defineMacro("_OPEN_DEFAULT");
+    Builder.defineMacro("_UNIX03_WITHDRAWN");
+    Builder.defineMacro("__370__");
----------------
hubert.reinterpretcast wrote:
> This is not defined by z/OS XL C/C++. It seems that this is more of a macro to be defined by a user application (perhaps as part of its configuration/port for z/OS) and less a macro that should be predefined by the compiler.
This is required for building libcxx, I'll add a comment here.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:750
+
+    if (Opts.C11 || Opts.GNUMode)
+      Builder.defineMacro("__IBM_UTF_LITERAL");
----------------
hubert.reinterpretcast wrote:
> Shouldn't UTF literals be reported as being enabled under strict C++11?
I will get back to you on this. I will need to investigate whether we are keeping this macro.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:753
+
+    if (Opts.C11 || (Opts.GNUMode && !Opts.CPlusPlus))
+      Builder.defineMacro("__IBMC_GENERIC");
----------------
hubert.reinterpretcast wrote:
> Is this consistent with `__has_extension` with respect to `-pedantic-errors`?
> That is, `-pedantic-errors` causes `__has_extension` to report the value that `__has_feature` would report. Compiler Explorer link: https://godbolt.org/z/EEn8rr
> 
> Same question for all of the other IBM-style feature test macros.
This macro is unused, so we are able to remove this.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:758
+      Builder.defineMacro("__DLL__");
+      // Macro __wchar_t exposes the definition of wchar_t data type
+      // in system headers.
----------------
hubert.reinterpretcast wrote:
> Should the comment instead say that `__wchar_t` should be defined so that the system headers do not try to declare `wchar_t` as a typedef?
You're right. I'll update the comment to be more accurate.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:761
+      Builder.defineMacro("__wchar_t");
+      Builder.defineMacro("_XOPEN_SOURCE", "600");
+    }
----------------
hubert.reinterpretcast wrote:
> Same comment as before re: macros that should be declared by the application.
Same as above: This is required for building libcxx.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:764
+
+    if (Opts.C11 || Opts.CPlusPlus11 || Opts.GNUMode) 
+      Builder.defineMacro("__IBMC_NORETURN");
----------------
hubert.reinterpretcast wrote:
> I don't see the relation between C++11 and `_Noreturn`. It's an extension in C++ that's available under, e.g., `-std=c++03`.
Thanks, I will remove CPlusPlus11.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:767
+
+    if (Opts.C11 || (Opts.GNUMode && Opts.CPlusPlus)) {
+      Builder.defineMacro("__IBM_CHAR16_T__");
----------------
hubert.reinterpretcast wrote:
> `char16_t` is not a keyword in C11, so `__IBM_CHAR16_T__` should not be defined for C. Same re: `char32_t`.
> 
> Also, `char16_t` and `char32_t` are indeed keywords in C++11.
Right, these seem to only require C++, so I will update the check. But I will also follow up on whether we plan to keep these macros here.


================
Comment at: clang/lib/Basic/Targets/OSTargets.h:770
+      Builder.defineMacro("__IBM_CHAR32_T__");
+      Builder.defineMacro("__IBMCPP_UTF_LITERAL__");
+    }
----------------
hubert.reinterpretcast wrote:
> The "IBMCPP" macro should not be defined in C modes.
See above: I will update this, but continue to investigate whether we need this macro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85324