build: const fmt formatter + suppress C4100 in this lib#41
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds compiler quality improvements and fmt library formatter support across six headers. Two methods mark unused parameters with ChangesCompiler Quality Annotations
fmt Formatter Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…nused]] - REL/Version.h: fmt::formatter<REL::Version>::format made const (fmt v11+/v12 call format() on a const formatter). - Actor.h SetLastRiddenMount, BSTSmartPointer.h Acquire: their unused parameters now use [[maybe_unused]] (matching this library's existing convention for no-op stubs) so they don't trip C4100 under /W4 /WX. No blanket /wd4100 suppression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
63afa08 to
0ed6f46
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CommonLibF4/include/REL/Version.h (1)
145-145: ⚡ Quick winInconsistency between AI summary and code; missing
constexpr.The AI summary states that
constexprwas added to this method, but the annotated code doesn't show theconstexprkeyword. Additionally, thestd::formatterspecialization at line 134 is markedconstexpr, but thisfmt::formatterversion is not. For consistency and to enable compile-time formatting, add theconstexprqualifier.Proposed fix
- auto format(const REL::Version& a_version, FormatContext& a_ctx) const + constexpr auto format(const REL::Version& a_version, FormatContext& a_ctx) const🤖 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 `@CommonLibF4/include/REL/Version.h` at line 145, The fmt::formatter specialization's member function auto format(const REL::Version& a_version, FormatContext& a_ctx) const is missing the constexpr qualifier causing inconsistency with the std::formatter specialization; mark this method constexpr to match the other specialization and allow compile-time formatting (update the fmt::formatter::format method signature to be constexpr auto format(const REL::Version& a_version, FormatContext& a_ctx) const).
🤖 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.
Nitpick comments:
In `@CommonLibF4/include/REL/Version.h`:
- Line 145: The fmt::formatter specialization's member function auto
format(const REL::Version& a_version, FormatContext& a_ctx) const is missing the
constexpr qualifier causing inconsistency with the std::formatter
specialization; mark this method constexpr to match the other specialization and
allow compile-time formatting (update the fmt::formatter::format method
signature to be constexpr auto format(const REL::Version& a_version,
FormatContext& a_ctx) const).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb529dbc-3fbc-4289-885f-544e1e3b1835
📒 Files selected for processing (3)
CommonLibF4/include/RE/Bethesda/Actor.hCommonLibF4/include/RE/Bethesda/BSTSmartPointer.hCommonLibF4/include/REL/Version.h
fmt v11+/v12 no longer falls back to std::formatter, so types that only had a std::formatter became unformattable via fmt/spdlog. Add fmt::formatter (guarded by FMT_VERSION) mirroring the std ones for BSFixedString, NiPoint2/3/4, NiMatrix3 and SEX. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Modernizes against the current toolchain/deps:
REL/Version.h: makefmt::formatter<REL::Version>::formatconst— fmt v12 requiresformatter::format() const.CommonLibF4/CMakeLists.txt: add/wd4100(unreferenced formal parameter) to this library's existing warning-suppression list, so it suppresses its own noisy warning under MSVC 14.4 rather than consumers suppressing globally.🤖 Generated with Claude Code
Summary by CodeRabbit