[Openmp-commits] [PATCH] D137543: [OpenMP][test] Add missing <cstdint> header to common.h

[Fangrui Song via Phabricator via Openmp-commits]

MaskRay requested changes to this revision.
MaskRay added inline comments.
This revision now requires changes to proceed.


================
Comment at: openmp/runtime/test/tasking/hidden_helper_task/common.h:9
 
 using kmp_int32 = int32_t;
 using kmp_int64 = int64_t;
----------------
xry111 wrote:
> Technically these should be `std::int32_t`, etc.  From the C++ standard:
> 
> ```
> All library entities except operator new and operator delete are defined within the namespace std or
> namespaces nested within namespace std. (footnote 166)
> 
> 166) The C standard library headers (Annex D.5) also define names within the global namespace, while the C++ headers for C
> library facilities (20.5.1.2) may also define names within the global namespace.
> ```
> 
> Note that it's a "may", not "must".
> 
> This diff is a portability improvement, so I think it's better to "do things correctly now" and add `std::`.
It is a "may" in the standard wording but the real world practice is that everything will break if you remove `::int32_t` from `cstdint`. All supported C++ standard libraries provide `::int32_t`.

I am not fussed about this difference, but cstdint needs to be ordered in the header list, fixing clang-format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137543