-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix clang-tidy warnings #468
Conversation
for more information, see https://pre-commit.ci
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
The HIP failure looks like an AMD compiler bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 25. Check the log or trigger a new build to see more.
There are lots of unexpected test failures. I'll dismiss the review and re-request a review when those are all fixed. |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
Ok, now the only remaining issue appears to be this ROCm compiler bug: In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_MultiFab.H:7:
In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_FArrayBox.H:6:
In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_BaseFab.H:19:
In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_Utility.H:15:
In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_Random.H:8:
In file included from /tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_RandomEngine.H:11:
In file included from /opt/rocm-5.7.0/include/hiprand/hiprand.hpp:35:
In file included from /opt/rocm-5.7.0/include/hiprand/hiprand_kernel.h:64:
/opt/rocm-5.7.0/include/hiprand/hiprand_kernel_hcc.h:128:19: error: reference to 'detail' is ambiguous
static_assert(detail::is_any_of<StateType,
^
/opt/rocm-5.7.0/include/hiprand/hiprand_kernel_hcc.h:68:11: note: candidate found by name lookup is 'detail'
namespace detail {
^
/tmp/bwibking/_work/1/s/extern/amrex/Src/Base/AMReX_GpuComplex.H:425:11: note: candidate found by name lookup is 'amrex::detail'
namespace detail
^ |
Merging this is blocked on fixing a Microphysics issue (due to an underlying ROCm issue): AMReX-Astro/Microphysics#1406 |
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
I am hoping this will also ensure the clang-tidy test passes for #368 |
We need Microphysics to avoid Also, I think your clang-tidy issue is unrelated (unfortunately). |
Hopefully, once #517 is merged, the AMD tests will also pass. |
…kka into BenWibking/clang-tidy-1
for more information, see https://pre-commit.ci
Depends on AMReX-Astro/Microphysics#1474. |
…kka into BenWibking/clang-tidy-1
for more information, see https://pre-commit.ci
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
AMREX_GPU_MANAGED GpuArray<Real, Physics_Traits<Channel>::numPassiveScalars> s_inflow{}; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) | ||
#if 0 // workaround AMDGPU compiler bug | ||
Real rho0 = NAN; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) | ||
Real u0 = NAN; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) |
Check notice
Code scanning / CodeQL
Short global name Note
#if 0 // workaround AMDGPU compiler bug | ||
Real rho0 = NAN; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) | ||
Real u0 = NAN; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) | ||
Real s0 = NAN; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) |
Check notice
Code scanning / CodeQL
Short global name Note
It's fixed now. |
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
Fixed missing |
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 5 pipeline(s). |
Description
This fixes many
clang-tidy
warnings thoughout the code. Most of them had to do with unnecessary header includes or const issues. No outputs should change at the bitwise level.Related issues
N/A
Checklist
Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an
x
inside the square brackets[ ]
in the Markdown source below:/azp run
.