Skip to content
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

Cleanup compiler bug workarounds and their comments #4362

Merged
merged 30 commits into from
Feb 6, 2024

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Feb 2, 2024

Prologue

While reviewing code and adding/removing compiler bug workarounds, I became increasingly aware of our inconsistent comments. Sometimes we have extremely helpful ^^^ workaround ^^^ vs. ^^^ no workaround ^^^ arrow comments, which makes it clear what code is "good", and what to do when removing the workarounds. Elsewhere, we have the older convention of just repeating the preprocessor condition (sometimes with arrows), which is fine in general but not for workarounds.

For example, we have some lines #if defined(__clang__) || defined(__EDG__) that are performing their traditional function of guarding desired test code that we're skipping for MSVC. But we also have lines of the exact same form that are guarding workaround machinery for separate Clang and EDG bugs! This is too confusing, so I went on a cleanup spree.

Along the way, I found a few compiler bug workarounds that were messed up, so I'm fixing those. These are called out under the "functional changes". I tried very hard to avoid making this a grab-bag PR, but I still wanted to clean up a few preprocessor lines for compiler bug workarounds, called out as "non-functional changes". There's a very small number of "miscellaneous comment cleanups", and then the rest is pure, highly systematic comment cleanups for the workaround arrows.

It might look like I mechanically performed these changes with a regex, but while I searched for them mechanically, I manually reviewed, categorized, and edited each of them. Manual changes with robotic precision - that's my trademark! 🤖

Preprocessor Functional Changes

  • Guard an MSVC workaround.
  • Allow EDG (in addition to Clang) to run test cases that only MSVC has trouble with.
  • Avoid a hidden "assert bug" scenario.
    • In C++23 mode, we should simply skip EDG instead of expecting it to give the wrong answer. This is VSO-1601179 "REPORTED: EDG's overload resolution incorrectly considers some ambiguous conversions to be unambiguous".

Preprocessor Non-Functional Changes

  • Guard with __cpp_lib_is_layout_compatible and __cpp_lib_is_pointer_interconvertible.
    • This has the same effect, and is still marked with // TRANSITION, LLVM-48860 as a reminder. I think this makes the #endif comment clearer, as it's separated by a huge distance and contains nested preprocessor logic. Checking the feature-test macros instead of the compiler also makes it clearer what to do in the future, even without "no workaround" to tell us.
  • We conventionally use the order defined(__clang__) || defined(__EDG__).
  • We conventionally use the order !defined(__clang__) && !defined(__EDG__).

Miscellaneous Comment Cleanups

  • Add missing arrows to a workaround comment.
  • Add missing arrows to other comments.
  • Attach TRANSITION comment.

Workaround Arrow Comment Cleanups

  • Mark feature-test macro definitions as no workaround.
    • This code is obviously desired.
  • Pair #else // ^^^ no workaround / workaround vvv with #endif // ^^^ workaround ^^^.
  • Pair #else // ^^^ workaround / no workaround vvv with #endif // ^^^ no workaround ^^^.
  • Pair #if !(defined(_DEBUG) && defined(__EDG__)) with #endif // ^^^ no workaround ^^^.
    • This guards desired test code that we're skipping for Debug EDG.
  • Pair #if !defined(_PREFAST_) && !defined(__EDG__) with #endif // ^^^ no workaround ^^^.
    • This guards desired test code that we're skipping for /analyze (aka Prefast) and EDG.
  • Pair #ifndef __EDG__ with #endif // ^^^ no workaround ^^^.
    • These are all guarding desired test code that we're skipping for EDG.
  • Pair #ifdef __EDG__ with #endif // ^^^ workaround ^^^.
    • This tuple_tester constructor works around an EDG bug with parenthesized aggregate initialization, and is undesired code.
  • Pair #if defined(__clang__) || defined(__EDG__) with #endif // ^^^ no workaround ^^^.
    • These are all guarding desired test code that we're skipping for MSVC.
  • Pair #if defined(__clang__) || defined(__EDG__) with #endif // ^^^ workaround ^^^.
    • All of this code is workaround machinery for separate Clang and EDG bugs.
  • Pair #if !defined(__clang__) && !defined(__EDG__) with #endif // ^^^ no workaround ^^^.
    • This guards desired test code that we're skipping for Clang and EDG due to separate bugs.
  • Pair #if !defined(__clang__) && !defined(__EDG__) with #endif // ^^^ workaround ^^^.
    • All of this code is MSVC-only workaround machinery.
  • Pair #ifndef _M_CEE with #endif // ^^^ no workaround ^^^.
    • These are all guarding desired test code that we're skipping for /clr.
  • Pair #ifndef _M_CEE_PURE with #endif // ^^^ no workaround ^^^.
    • This guards desired test code that we're skipping for /clr:pure.
  • Pair #ifdef __cplusplus_winrt with #endif // ^^^ workaround ^^^.
    • This guards a C++/CX workaround, which is undesired code.
  • Pair #ifndef __clang__ with #endif // ^^^ no workaround ^^^.
    • All of this guards desired code that we're skipping for Clang.
  • Pair #if !(defined(__clang__) && defined(_M_IX86)) with #endif // ^^^ no workaround ^^^.
    • All of this guards desired code that we're skipping for Clang x86.
  • Pair #ifdef __clang__ with #endif // ^^^ workaround ^^^.
    • All of this guards Clang-only workarounds, which are undesired code.
  • Pair #ifndef __SANITIZE_ADDRESS__ with #endif // ^^^ no workaround ^^^.
    • This guards desired test code that we're skipping for ASan.
  • Pair defined(_MSVC_EXECUTION_CHARACTER_SET) with #endif // ^^^ no workaround ^^^.
    • This is desired code that's waiting for compiler support.
  • Drop <algorithm> comment change to avoid conflicting with ADL-proof implementation of remaining sorting-related algorithms #4367's removal of that line.

Epilogue

  • Perhaps surprisingly, there's only one trivial adjacent-edit merge conflict with Enable __cpp_lib_concepts for EDG, part 3 #4298, which I will easily resolve when landing these PRs.
  • There are a few lines that I didn't mess with for various reasons (e.g. to avoid conflicts with in-flight prod/be changes).
  • Along the way, I noticed that a few compiler bugs have been resolved as fixed. I'll deal with them during the next toolset update, which will be able to remove even more compiler bug workarounds.

In C++23 mode, we should simply skip EDG instead of expecting it to give the wrong answer.

This is VSO-1601179 "REPORTED: EDG's overload resolution incorrectly considers some ambiguous conversions to be unambiguous".
…cpp_lib_is_pointer_interconvertible`.

This has the same effect, and is still marked with `// TRANSITION, LLVM-48860` as a reminder.

I think this makes the `#endif` comment clearer, as it's separated by a huge distance and contains nested preprocessor logic.

Checking the feature-test macros instead of the compiler also makes it clearer what to do in the future, even without "no workaround" to tell us.
… no workaround ^^^`.

This guards desired test code that we're skipping for Debug EDG.
…^^ no workaround ^^^`.

This guards desired test code that we're skipping for `/analyze` (aka Prefast) and EDG.
These are all guarding desired test code that we're skipping for EDG.
This `tuple_tester` constructor works around an EDG bug with parenthesized aggregate initialization, and is undesired code.
… no workaround ^^^`.

These are all guarding desired test code that we're skipping for MSVC.
… workaround ^^^`.

All of this code is workaround machinery for separate Clang and EDG bugs.
…^^ no workaround ^^^`.

This guards desired test code that we're skipping for Clang and EDG due to separate bugs.
…^^ workaround ^^^`.

All of this code is MSVC-only workaround machinery.
These are all guarding desired test code that we're skipping for `/clr`.
This guards desired test code that we're skipping for `/clr:pure`.
This guards a C++/CX workaround, which is undesired code.
All of this guards desired code that we're skipping for Clang.
…^^^ no workaround ^^^`.

All of this guards desired code that we're skipping for Clang x86.
All of this guards Clang-only workarounds, which are undesired code.
… ^^^`.

This guards desired test code that we're skipping for ASan.
…workaround ^^^`.

This is desired code that's waiting for compiler support.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 2, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 2, 2024 15:20
frederick-vs-ja added a commit to frederick-vs-ja/STL that referenced this pull request Feb 3, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 5, 2024
@StephanTLavavej
Copy link
Member Author

I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member Author

I've pushed a merge with main to resolve a trivial adjacent-edit merge conflict with #4298 in tests/std/tests/GH_001596_adl_proof_algorithms/test.compile.pass.cpp.

@StephanTLavavej StephanTLavavej merged commit f49ffd2 into microsoft:main Feb 6, 2024
35 checks passed
@StephanTLavavej StephanTLavavej deleted the workaround-comments branch February 6, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants