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

Toolset update: VS 2022 17.12 Preview 2, Clang 18.1.8 #4947

Merged
merged 30 commits into from
Sep 12, 2024

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Sep 11, 2024

Fixes #1586.

📜 Changelog

  • Code cleanups:
    • Removed compiler bug workarounds.
  • Infrastructure improvements:
    • Updated dependencies.
      • Updated build compiler to VS 2022 17.12 Preview 2.
      • Updated Clang to 18.1.8 (now required).
      • Updated Python to 3.12.6.

📝 Code Format Overhaul

clang-format 18 has noticeably improved its handling of concepts and variable templates. I was able to remove lots of forced wrapping and complete suppression, while needing very few manual adjustments.

As always, the goal is to rely on clang-format to achieve consistency and general readability, not pixel-perfection. I reviewed all of the regen changes, and manually adjusted the ones I thought were obnoxious. Similarly, I removed clang-format suppression when I thought the result was reasonable, even if it was a significant appearance change. We should strive to rely on clang-format for the vast majority of the codebase, giving it only gentle nudges with forced wrapping, and resorting to suppression only when it wants to do something egregious or we need to do something specific for readability or to stay in sync with external sources.

I also explicitly documented my clang-format update process, so we have one less tacit set of steps to perform.

⚙️ Commits

  • PowerShell 7.4.5.
  • Python 3.12.6.
  • New pool.
  • VS 2022 17.12 Preview 2. (Still not updating yvals_core.h because the internal toolset has not yet been updated)
  • Clang 18.1.8 (we now require Clang 18).
  • Remove workarounds for VSO-2170500 (C1XX type trait optimization).
  • [Clang][concepts] Conditional explicit specifier is checked too early in constrained constructor llvm/llvm-project#59827 was fixed by Clang 18.
  • Request LLVM to implement _CountLeadingZeros and _CountLeadingZeros64 when targeting ARM/ARM64 #1586 was fixed by Clang 18.
  • Clang 18 understands -Wno-overriding-option, so drop -Wno-unknown-warning-option -Wno-overriding-t-option.
  • Clang 18 understands -Wno-nan-infinity-disabled, so drop -Wno-unknown-warning-option.
  • Add 'TRANSITION, Clang 20' comment noting that __builtin_isgreater etc. will be constexpr.
  • static lambda call operator is not convertible to function pointer on win32 llvm/llvm-project#62594 was fixed by Clang 18.
  • Clang 18 fixed a bug with static call operators.
  • Expect clang-format 18.1.8.
  • Update .clang-format.
    • Comment my .clang-format update process.
    • Accept defaults, except for:
      • Change BreakAfterAttributes from Leave to Never (the old default).
  • Regen clang-format, no manual changes.
  • Change AllowBreakBeforeNoexceptSpecifier from Never to OnlyWithParen.
  • Regen clang-format, no manual changes.
  • LLVM-51935 clang-format-13 crashes with AlignArrayOfStructures llvm/llvm-project#51277 was fixed a while ago, but AlignArrayOfStructures appears to be more trouble than it's worth.
  • Manual: Add parens to avoid spaces around arrow operator.
  • Manual: Detach comment.
  • Manual: Add forced wrapping before trailing return types.
  • Manual: Remove forced wrapping.
  • Manual: Remove lots of clang-format suppression.
  • Manual: Remove clang-format suppression and introduce parens.
  • Manual: Shrink clang-format suppression in <concepts> down to concept common_with.
  • Comment: Mention <Windows.h> with correct capitalization.
  • Overhaul header sorting.
    • Use BASIC-style priorities: 10 for yvals/yvals_core, 20 for normal headers, 30 for Windows headers (with fine-grained SortPriority), and 40 for test headers.
    • Mention (yvals|yvals_core) to be more searchable.
    • userenv.h wasn't being included (anymore?).
    • winioctl.h should be spelled lowercase.
    • Make the regex for __msvc_MEOW.hpp clearer (we have no other double-underscore headers) and mention it in priority order as there's no chance it'll grab matches first.
    • Add a rule to sort initguid.h first, similar to how winioctl.h sorts last.
  • Add newlines to save one column and stay within 120.

😻 Help wanted!

✅ STL-ASan-CI passed

On the first try! https://dev.azure.com/vclibs/STL/_build/results?buildId=17504&view=results

StephanTLavavej and others added 29 commits September 10, 2024 15:04
…warning-option -Wno-overriding-t-option`.

GH 4452 originally added this.
Comment my .clang-format update process.

Accept defaults, except for:

* Change BreakAfterAttributes from Leave to Never (the old default).
Use BASIC-style priorities: 10 for yvals/yvals_core, 20 for normal headers, 30 for Windows headers (with fine-grained SortPriority), and 40 for test headers.

Mention (yvals|yvals_core) to be more searchable.

userenv.h wasn't being included (anymore?).

winioctl.h should be spelled lowercase.

Make the regex for __msvc_MEOW.hpp clearer (we have no other double-underscore headers) and mention it in priority order as there's no chance it'll grab matches first.

Add a rule to sort initguid.h first, similar to how winioctl.h sorts last.
…d `_Adjacent_transform_constraints` that we thought was LLVM-61763.
@StephanTLavavej StephanTLavavej added the infrastructure Related to repository automation label Sep 11, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner September 11, 2024 09:16
@StephanTLavavej

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clang-format improvements are really nice. I still hate what it does with defaulted or deleted special member functions with trailing-requires-clauses, but almost every other change is an improvement.

@@ -97,7 +97,8 @@ struct _Default_allocate_traits {
#ifdef __clang__ // Clang and MSVC implement P0784R7 differently; see GH-1532
_CONSTEXPR20
#endif // defined(__clang__)
void* _Allocate(const size_t _Bytes) {
void*
_Allocate(const size_t _Bytes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two occurrences are unfortunate, but not worth blocking the PR over. I'll experiment and see if I can find an improvement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, thanks.

@CaseyCarter CaseyCarter removed their assignment Sep 11, 2024
@StephanTLavavej StephanTLavavej self-assigned this Sep 11, 2024
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej StephanTLavavej merged commit 6cb175d into microsoft:main Sep 12, 2024
39 checks passed
@StephanTLavavej StephanTLavavej deleted the vs17.12p2 branch September 12, 2024 03:13
@technic
Copy link

technic commented Sep 29, 2024

Hi, do you maintain a list where it is specified which toolset version requires which minimum clang version? I dont want to get errors like this "Unexpected compiler version, expected Clang 18.0.0 or newer." all of a sudden.

@CaseyCarter
Copy link
Member

Hi, do you maintain a list where it is specified which toolset version requires which minimum clang version? I dont want to get errors like this "Unexpected compiler version, expected Clang 18.0.0 or newer." all of a sudden.

We didn't, but I've just scraped the history and added the required Clang version (note that it's always the Clang that ships with Visual Studio) to the notes in https://github.com/microsoft/STL/wiki/Macro-_MSVC_STL_UPDATE, at least going back as far as we have GitHub history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to repository automation
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Request LLVM to implement _CountLeadingZeros and _CountLeadingZeros64 when targeting ARM/ARM64
4 participants