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

Various cleanups: Test code #4122

Merged

Conversation

StephanTLavavej
Copy link
Member

Usual disclaimer about how we dislike grab-bag PRs as a general rule, and how we avoid large-scale cleanups of test code (low value, potentially disrupts the weird subtle things that the tests were trying to cover). The existence of this PR means that I think these cases are worth it for codebase consistency and that they're low-risk.

  • format.py: import copy was unused.
    • Thanks to Pylance for finding this.
  • Style: constexpr friend => friend constexpr
    • This was the only place that wasn't following our convention.
  • Style: Nameless value parameter const choose => choose
  • Drop unnecessary static for namespace-scope permissive().
    • This one was easy to miss, because it's followed by a static member function.
  • Test cleanup: Catch by const ref, even when unnamed.
    • Catching by modifiable reference is weird; there are a couple of places elsewhere where we're intentionally doing so, but here we certainly don't need to. As usual, following the normal pattern in most places helps to draw attention to the weird occurrences.
  • Test cleanup: Catch by const ref.
    • Here we're just observing the filesystem_error.
  • P0811R3_midpoint_lerp: static inline constexpr => static constexpr
    • I suspect that the inline was accumulated before a compiler bug workaround was added.
  • VSO_0000000_vector_algorithms: Drop inline after template.
    • We conventionally never say inline here.
  • Tests: Add bug databases to function names.
    • It's very important to cite bug databases and not just plain numbers (as we've gone through so many databases over the years). These occurrences were at least properly commented, but we conventionally name functions with bug databases too.
    • Example internal URLs (one must be VPNed to access these): Dev10-391805 and DevDiv-1184701.
  • Tests: Drop unnecessary std:: qualification.
    • I consider these to be "slam dunks". On the other hand, we accumulated a bunch of std::move and std::forward during an annoying era where Clang was warning about unqualified calls before we silenced that; I don't think it's worth the effort to go clean them up.
  • P0083R3_splicing_maps_and_sets: #ifdef __cpp_constinit => #if _HAS_CXX20
    • Once compiler divergence has subsided and there are no escape hatches, we prefer to use the coarse-grained modes to guard things.

@StephanTLavavej StephanTLavavej added the test Related to test code label Oct 23, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner October 23, 2023 16:30
@StephanTLavavej StephanTLavavej self-assigned this Oct 24, 2023
@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 f7c3cb6 into microsoft:main Oct 25, 2023
37 checks passed
@StephanTLavavej StephanTLavavej deleted the dry-cleaning-6-test-cleanups branch October 25, 2023 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants