This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][test] Add #include <cstdint> for gcc-13
ClosedPublic

Authored by Ami-zhang on Nov 7 2022, 5:04 AM.

Diff Detail

Event Timeline

Ami-zhang created this revision.Nov 7 2022, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 7 2022, 5:04 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
Ami-zhang requested review of this revision.Nov 7 2022, 5:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
xry111 added inline comments.Nov 7 2022, 5:22 AM
openmp/runtime/test/tasking/hidden_helper_task/common.h
9

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::.

SixWeining retitled this revision from [Support] Add missing <cstdint> header to common.h to [OpenMP][test] Add missing <cstdint> header to common.h.Nov 7 2022, 5:38 AM

I just revised the patch title.

MaskRay requested changes to this revision.Nov 7 2022, 9:46 AM
MaskRay added inline comments.
openmp/runtime/test/tasking/hidden_helper_task/common.h
9

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.

This revision now requires changes to proceed.Nov 7 2022, 9:46 AM

I just revised the patch title.

Thanks a lot.

Ami-zhang added inline comments.Nov 7 2022, 5:28 PM
openmp/runtime/test/tasking/hidden_helper_task/common.h
9

Thanks.
It's done.

Ami-zhang updated this revision to Diff 473835.Nov 7 2022, 5:29 PM

Addressed @MaskRay's comment.

xen0n retitled this revision from [OpenMP][test] Add missing <cstdint> header to common.h to [OpenMP][test] Add #include <cstdint> for gcc-13.Nov 9 2022, 12:42 AM
xen0n edited the summary of this revision. (Show Details)

I've revised the patch title and summary for you; it's trivial and self-explaining enough to not need the error output and verbose explanation.

I've revised the patch title and summary for you; it's trivial and self-explaining enough to not need the error output and verbose explanation.

Thank you.

MaskRay accepted this revision.Nov 9 2022, 9:26 AM

LGTM.

This revision is now accepted and ready to land.Nov 9 2022, 9:26 AM
This revision was automatically updated to reflect the committed changes.