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.4 Preview 3, Clang 15 #3155

Merged
merged 37 commits into from
Oct 14, 2022

Conversation

StephanTLavavej
Copy link
Member

As usual, this is structured as a series of fine-grained commits for reviewing convenience.

  • @CaseyCarter's commit to disable Clang 15's -Wunqualified-std-cast-call in testing, from Disable Clang 15's -Wunqualified-std-cast-call in testing #3149.
  • Fix divergence between tests/libcxx/expected_results.txt and skipped_tests.txt, plus minor cleanups:
    • Add spaces at the start of comments.
    • Fix newline divergence.
    • Fix divergence: This doesn't appear to be an x64 vs. ARM issue at all.
    • Fix divergence: This was already mentioned under "generate_feature_test_macro_components.py needs to learn about C1XX".
    • Convention: Change AMD64 to x64. Also X86 to x86.
  • azure-devops/create-1es-hosted-pool.ps1: Drop SimplySecureV2OptInTag.
  • Python 3.10.8.
  • New pool, containing VS 2022 17.4 Preview 3 with Clang 15, and Patch Tuesday.
  • README.md: 17.4 Preview 3.
  • yvals_core.h: Require Clang 15.
  • P1208R6_source_location/test.cpp: Remove workarounds now that MSVC-PR-418361 has shipped; see Update P1208R6_source_location for a compiler fix #3046.
  • xcharconv_tables.h: Remove workarounds for VSO-1579484 "Standard Library Header Units: charconv refactoring + topo sort = error LNK2019: unresolved external symbol".
  • xstring: Update citation: VSO-1601168 "REPORTED: EDG's __builtin_memcmp emits bogus errors with move-constructed strings and constexpr dynamic allocations" was fixed, VSO-1641993 "REPORTED: EDG __builtin_memcmp misbehavior, part 2" remains.
  • Update .clang-format defaults for Clang 15.
    • As usual, this is obtained by running clang-format --style=llvm --dump-config to get the defaults, which we record as comments, so it's clear what behavior we're customizing.
    • Note that in addition to renaming IndentRequires to IndentRequiresClause, they changed the default setting to true, so we no longer need to customize it.
  • Regen clang-format, no manual changes.
    • clang-format 15 is much better at handling concepts and variable templates, making progress on Report clang-format issues #2475.
    • I manually reviewed these changes, leading to...
  • Manually fix formatting.
  • .clang-format: Set AfterRequiresInClause: true.
    • This new option allows us to precisely express our preferred convention: requires clauses get a space, but requires expressions don't (IIRC because they resemble functions).
  • Regen clang-format, no manual changes.
  • .clang-format: Set InsertBraces: true.
    • This new option should make life easier for new contributors. The documentation warns that clang-format's lack of complete semantic information means that its brace changes should always be reviewed by humans, but (1) we are already perfectly consistent about braces (with one exception, see next commit) so this results in no changes to existing code, and (2) we already review all incoming code for adherence to brace discipline.
  • Regen clang-format, no manual changes.
    • I had forgotten that I intentionally skipped tr1, our favorite legacy test suite, when manually adding braces to the entire codebase (it was a lot of typing). We still avoid cleaning up tr1 to adhere to modern conventions (e.g. it still has multiple declarations on a single line, and lots of other practices we avoid), but it's easy to let the tool make these changes so everything is consistent. I couldn't resist one thing...
  • Add newlines.
    • Since clang-format is adding braces to control flow, we should at least add newlines to separate non-chained if statements.
  • Add more newlines.
    • Found other occurrences in the codebase.
  • Then, we can remove a ton of clang-format suppression and empty comments to force wrapping. Organized in increasing order of complexity (I stopped at moderate-scale reflows - more clang-format suppression could be removed, but this is already a big PR):
    • Remove clang-format suppression, literally no changes.
    • Shrink clang-format suppression, literally no changes.
    • Remove clang-format suppression, no changes other than a newline.
    • Remove/shrink clang-format suppression, only indentation changes.
    • Remove clang-format suppression, aligns backslashes.
    • Remove clang-format suppression, reasonable reflows.
    • Remove unnecessary empty comments //, literally no changes.
  • And one last cleanup: Move binary operators to the beginning of lines.

CaseyCarter and others added 30 commits October 12, 2022 07:53
... except for the "include each header alone" test. It covers only product code, which should not trigger this warning.

Fixes GH 3134.
…ature_test_macro_components.py needs to learn about C1XX".
…arconv refactoring + topo sort = error LNK2019: unresolved external symbol".
@StephanTLavavej StephanTLavavej added high priority Important! infrastructure Related to repository automation labels Oct 13, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 13, 2022 06:29
@StephanTLavavej StephanTLavavej added this to Final Review in Code Reviews Oct 13, 2022
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.

Note that StephanTLavavej#26 addresses my requested changes (I didn't simply push into the branch to avoid interfering with the ongoing pipeline investigation.)

stl/inc/ranges Outdated Show resolved Hide resolved
tests/std/include/range_algorithm_support.hpp Outdated Show resolved Hide resolved
tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp Outdated Show resolved Hide resolved
Code Reviews automation moved this from Final Review to Work In Progress Oct 13, 2022
@CaseyCarter CaseyCarter moved this from Work In Progress to Ready To Merge in Code Reviews Oct 13, 2022
@CaseyCarter CaseyCarter moved this from Ready To Merge to Final Review in Code Reviews Oct 13, 2022
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Oct 13, 2022
@StephanTLavavej
Copy link
Member Author

I've pushed a commit to fix /analyze warnings when building tzdb.cpp:

stl\src\tzdb.cpp(380) : warning C28193: 'Temp_value_#1604' holds a value that must be examined.
stl\src\tzdb.cpp(386) : warning C28193: 'Temp_value_#1610' holds a value that must be examined.

/analyze didn't like the fact that we were assigning the result of new (nothrow) to a data member, even though we immediately inspected that data member. I've restructured this to use a local variable (scoped to the if-statement with C++17; this is OK since tzdb.cpp is C++20), so /analyze can see that we're examining it, without needing to suppress the warning.

There is no behavioral change to slightly delaying the data member assignment - the _Info is a local unique_ptr, so if we have to return nullptr;, the user won't see that we didn't assign _Info->MEOW - and _Info holds __std_tzdb_time_zones_info{} so the data members are already null.

@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 2f8342a into microsoft:main Oct 14, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 14, 2022
@StephanTLavavej StephanTLavavej deleted the vs-17.4p3 branch October 14, 2022 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority Important! infrastructure Related to repository automation
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

test: hundreds of failures with Clang 15.0.1
4 participants