psyqo: use portable inline and no_unique_address spellings#2049
psyqo: use portable inline and no_unique_address spellings#2049nicolasnoble wants to merge 1 commit into
Conversation
muteSpu() used the GNU __inline__ spelling, which MSVC rejects; plain static inline is standard C99/C++ and identical on GCC. The Vector members used the standard [[no_unique_address]], which the MSVC ABI (MSVC and clang-cl) ignores, so wrap it in a PSYQO_NO_UNIQUE_ADDR macro that feature-detects [[msvc::no_unique_address]] via __has_cpp_attribute and falls back to the standard spelling everywhere else. No change on the GCC toolchain. Co-authored-by: Elias Daler <eliasdaler@protonmail.com> Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
📝 WalkthroughWalkthroughThis PR makes two independent compiler compatibility fixes: it changes the ChangesCompiler Compatibility Fixes
Estimated code review effort: 1 (Trivial) | ~5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mips/psyqo/vector.hh (1)
35-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider distinguishing clang-cl from regular Clang.
__has_cpp_attribute(msvc::no_unique_address)returns true on all Clang targets, not just clang-cl. On regular Clang targeting the Itanium ABI (e.g., Linux), this macro would select[[msvc::no_unique_address]]instead of the standard[[no_unique_address]]. These two attributes follow different ABI rules for empty member elision, which could changeVector's layout. The static_asserts at lines 245-247 provide a compile-time safety net, but a more precise check avoids the risk entirely.♻️ Suggested more precise macro
-#if __has_cpp_attribute(msvc::no_unique_address) -#define PSYQO_NO_UNIQUE_ADDR [[msvc::no_unique_address]] -#else -#define PSYQO_NO_UNIQUE_ADDR [[no_unique_address]] -#endif +#if defined(_MSC_VER) || defined(__clang_cl__) +#define PSYQO_NO_UNIQUE_ADDR [[msvc::no_unique_address]] +#elif __has_cpp_attribute(no_unique_address) +#define PSYQO_NO_UNIQUE_ADDR [[no_unique_address]] +#else +#define PSYQO_NO_UNIQUE_ADDR +#endif🤖 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/mips/psyqo/vector.hh` around lines 35 - 41, The PSYQO_NO_UNIQUE_ADDR selection in vector.hh is too broad because __has_cpp_attribute(msvc::no_unique_address) matches regular Clang too, not just clang-cl. Update the preprocessor check around PSYQO_NO_UNIQUE_ADDR to distinguish clang-cl/MSVC ABI from regular Clang so Vector uses [[msvc::no_unique_address]] only when targeting the MSVC ABI and otherwise falls back to [[no_unique_address]]; keep the existing static_asserts as a safeguard.
🤖 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 `@src/mips/psyqo/vector.hh`:
- Around line 35-41: The PSYQO_NO_UNIQUE_ADDR selection in vector.hh is too
broad because __has_cpp_attribute(msvc::no_unique_address) matches regular Clang
too, not just clang-cl. Update the preprocessor check around
PSYQO_NO_UNIQUE_ADDR to distinguish clang-cl/MSVC ABI from regular Clang so
Vector uses [[msvc::no_unique_address]] only when targeting the MSVC ABI and
otherwise falls back to [[no_unique_address]]; keep the existing static_asserts
as a safeguard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 60f6d848-e195-4bde-b541-3e75291f386e
📒 Files selected for processing (2)
src/mips/common/hardware/spu.hsrc/mips/psyqo/vector.hh
1 participant
muteSpu() used the GNU inline spelling, which MSVC rejects; plain
static inline is standard C99/C++ and identical on GCC. The Vector members
used the standard [[no_unique_address]], which the MSVC ABI (MSVC and
clang-cl) ignores, so wrap it in a PSYQO_NO_UNIQUE_ADDR macro that
feature-detects [[msvc::no_unique_address]] via __has_cpp_attribute and
falls back to the standard spelling everywhere else. No change on the GCC
toolchain.