[Tests] avoid including sycl.hpp in tests - #1977
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change adds missing standard and SYCL includes across framework and test sources. It also changes ChangesHeader and assertion updates
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shamtest/details/TestAssertList.hpp`:
- Line 120: Update TestAssertList.hpp around assert_float_equal to include
<cmath> directly before using std::fabs, since shambase/aliases_float.hpp only
provides aliases and should not be relied on for that declaration. Keep the
existing std::fabs-based comparison unchanged, and make the header
self-sufficient so it does not depend on a transitive include.
In `@src/tests/shammodels/gsph/GSPHIntegrationTests.cpp`:
- Line 23: Remove the unused shambackends/sycl.hpp include from
GSPHIntegrationTests.cpp, leaving the test’s standard library includes and
implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8a75768-dc08-40a2-b5e1-b461acb58c8c
📒 Files selected for processing (6)
src/shamtest/details/TestAssertList.hppsrc/shamtest/shamtest.hppsrc/tests/shambase/ConstantsTests.cppsrc/tests/shamformat/format_tests.cppsrc/tests/shammodels/gsph/GSPHIntegrationTests.cppsrc/tests/shamsys/LogTests.cpp
| inline void assert_float_equal( | ||
| std::string assert_name, f64 a, f64 b, f64 eps, SourceLocation loc = SourceLocation{}) { | ||
| f64 diff = sycl::fabs(a - b); | ||
| f64 diff = std::fabs(a - b); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '`#include`.*cmath|std::fabs|fabs' \
src/shamtest/details/TestAssertList.hpp \
src/shambase/aliases_float.hppRepository: Shamrock-code/Shamrock
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Candidate include/search files:\n'
git ls-files | rg '(^|/)(TestAssertList\.hpp|aliases_float\.hpp|.*float.*\.hpp|.*assert.*\.hpp)$' || true
printf '\nIncludes in TestAssertList.hpp:\n'
sed -n '1,160p' src/shamtest/details/TestAssertList.hpp
printf '\nSearch for aliases_float.hpp and cmath include chain:\n'
rg -n 'aliases_float|<cmath>|<cmath>|fabs|std::fabs|stl' --glob '*.{hpp,h,hxx,cpp,cc}' .Repository: Shamrock-code/Shamrock
Length of output: 26476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'aliases_float.hpp:\n'
cat -n src/shambase/include/shambase/aliases_float.hpp
printf '\nfloats.hpp:\n'
cat -n src/shambase/include/shambase/floats.hpp
printf '\nDirect includes of cmath by shambase aliases_float transitive chain candidates:\n'
rg -n -U '(^|[^:])(`#include`\s*<[^>]*cmath>|^//\s*.*cmath|std::fabs|fabs)' src/shambase/include/shambase/aliases_float.hppRepository: Shamrock-code/Shamrock
Length of output: 2699
Include <cmath> for std::fabs.
TestAssertList.hpp uses std::fabs, but shambase/aliases_float.hpp only defines numeric aliases and does not include <cmath>. Add <cmath> directly so assert_float_equal does not depend on a transitive include.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/shamtest/details/TestAssertList.hpp` at line 120, Update
TestAssertList.hpp around assert_float_equal to include <cmath> directly before
using std::fabs, since shambase/aliases_float.hpp only provides aliases and
should not be relied on for that declaration. Keep the existing std::fabs-based
comparison unchanged, and make the header self-sufficient so it does not depend
on a transitive include.
| * - Edge cases (zero acceleration, constant force) | ||
| */ | ||
|
|
||
| #include "shambackends/sycl.hpp" |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '`#include`.*sycl|sycl::|sycl/' \
src/shambackends/sycl.hpp \
src/tests/shammodels/gsph/GSPHIntegrationTests.cppRepository: Shamrock-code/Shamrock
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching sycl.hpp:"
git ls-files | grep -E '(^|/)sycl\.hpp$|GSPHIntegrationTests\.cpp$' || true
echo
echo "Tracked files containing shambackends/sycl.hpp:"
rg -n 'include\s+"?shambackends/sycl.hpp"?|sycl::|sycl/' . || trueRepository: Shamrock-code/Shamrock
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate wrapper files:"
git ls-files | grep -E '(^|/)shambackends/sycl\.hpp$' || true
echo
echo "Exact tracked include definitions:"
rg -n 'define SHAMROCK.*SYCL|include <sycl|include "sycl|namespace shambackends|sycl::' $(git ls-files | grep -E '\.(h|hpp|cc|cpp)$' | grep -v node_modules) | head -200 || true
echo
echo "GSPH test include block:"
sed -n '1,60p' src/tests/shammodels/gsph/GSPHIntegrationTests.cpp
echo
echo "GSHP test direct sycl usages only:"
rg -n 'sycl::|shambackends/sycl|"sycl' src/tests/shammodels/gsph/GSPHIntegrationTests.cpp || trueRepository: Shamrock-code/Shamrock
Length of output: 24606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "shambackends/sycl.hpp:"
cat -n src/shambackends/include/shambackends/sycl.hpp
echo
echo "shambackends include directory contents:"
git ls-files src/shambackends/include/shambackends | sort
echo
echo "GSHP test tail:"
wc -l src/tests/shammodels/gsph/GSPHIntegrationTests.cpp
tail -n 120 src/tests/shammodels/gsph/GSPHIntegrationTests.cpp
echo
echo "Any direct sycl include or use from GSPHIntegrationTests.cpp:"
python3 - <<'PY'
from pathlib import Path
p = Path("src/tests/shammodels/gsph/GSPHIntegrationTests.cpp")
text = p.read_text()
for i,line in enumerate(text.splitlines(),1):
if "sycl" in line:
print(f"{i}: {line}")
PYRepository: Shamrock-code/Shamrock
Length of output: 8049
Remove the unused SYCL backend include.
GSPHIntegrationTests.cpp only uses standard library types and includes shambackends/sycl.hpp, which exports <sycl/sycl.hpp>. The test has no sycl usage, so drop this include.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/shammodels/gsph/GSPHIntegrationTests.cpp` at line 23, Remove the
unused shambackends/sycl.hpp include from GSPHIntegrationTests.cpp, leaving the
test’s standard library includes and implementation unchanged.
|
@Mergifyio queue |
Merge Queue Status
Waiting for any of
All conditions
|
|
@Mergifyio queue |
☑️ Command
|
Workflow reportworkflow report corresponding to commit 41018af Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@Mergifyio queue |
☑️ Command
|
2 participants
Shave off around 30 sec of build wall time on my Mac
393s of sycl.hpp header cost -> 361s