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

[Limin Zhang via Phabricator via Openmp-commits]

Ami-zhang added inline comments.


================
Comment at: openmp/runtime/test/tasking/hidden_helper_task/common.h:9
 
 using kmp_int32 = int32_t;
 using kmp_int64 = int64_t;
----------------
MaskRay wrote:
> 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.
Thanks.
It's done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137543