Merge fallthrough to cpp #1052
Merge fallthrough to cpp #1052
Conversation
Since GCC version 4.9.3 does not support the -Wimplicit-fallthrough warning option or the __attribute__((fallthrough)) annotation, introduce the corresponding fixes to prevent build errors.
…tHuangGarmin-master
There was a problem hiding this comment.
Pull request overview
This PR modernizes fallthrough handling in tinyxml2 by replacing comment-based annotations with a compiler-aware TIXML_FALLTHROUGH macro. The change provides better compiler support for suppressing fallthrough warnings across different platforms (MSVC, GCC, Clang) and C++ standards (C++17's [[fallthrough]], compiler-specific attributes, or fallback to no-op).
Key Changes:
- Adds a portable
TIXML_FALLTHROUGHmacro with detection logic for C++17 attributes, Clang-specific attributes, GCC attributes, and MSVC compatibility - Replaces three comment-based fallthrough annotations (
//fall through) with the new macro in the UTF-32 to UTF-8 conversion function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tinyxml2.cpp
Outdated
| #ifdef __cplusplus | ||
| # ifndef __has_cpp_attribute | ||
| # define __has_cpp_attribute(x) 0 | ||
| # endif | ||
| #else | ||
| # ifndef __has_c_attribute | ||
| # define __has_c_attribute(x) 0 | ||
| # endif | ||
| #endif | ||
|
|
||
| #ifdef __cplusplus | ||
| # if defined(_MSC_VER) | ||
| # define TIXML_FALLTHROUGH (void(0)) | ||
| # elif (__cplusplus >= 201703L && __has_cpp_attribute(fallthrough)) | ||
| # define TIXML_FALLTHROUGH [[fallthrough]] | ||
| # elif __has_cpp_attribute(clang::fallthrough) | ||
| # define TIXML_FALLTHROUGH [[clang::fallthrough]] | ||
| # elif __has_attribute(fallthrough) | ||
| # define TIXML_FALLTHROUGH __attribute__((fallthrough)) | ||
| # else | ||
| # define TIXML_FALLTHROUGH (void(0)) | ||
| # endif | ||
| #else | ||
| # if defined(_MSC_VER) | ||
| # define TIXML_FALLTHROUGH (void(0)) | ||
| # elif __has_c_attribute(fallthrough) | ||
| # define TIXML_FALLTHROUGH [[fallthrough]] | ||
| # elif __has_attribute(fallthrough) | ||
| # define TIXML_FALLTHROUGH __attribute__((fallthrough)) | ||
| # else | ||
| # define TIXML_FALLTHROUGH (void(0)) | ||
| # endif | ||
| #endif |
There was a problem hiding this comment.
[nitpick] The C-specific code paths (checking __has_c_attribute) are unnecessary in a .cpp file, which is always compiled as C++. The #ifdef __cplusplus condition will always be true in this context. Consider removing the C-specific fallback code (lines 43-47 and 61-70) to simplify maintenance, or add a comment explaining if this code is intended to be portable to other contexts.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@leethomason I've opened a new pull request, #1053, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
AI had a good suggestion, just screwed up implementation. Fixing... |
No description provided.