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

Cleanups: Formatting and comments #3523

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Mar 3, 2023

  • Avoid // strengthened putting { on the next line. This is one of the few scenarios where we prefer to use C-style comments.
  • Remove double newlines in product code, both within functions (where they were especially distracting) and outside of functions.
    • This would be a low-value cleanup to apply to test code, which I am intentionally not doing.
    • In theory, double newlines can be used to create breaks between distinct groups of code. In practice, we've used them increasingly rarely and inconsistently, so they're more of a distraction than anything useful.
    • I am leaving a few double newlines that are separating license banners.
  • Thanks @JMazurkiewicz for noticing that I was modifying generated code! 😻 I've updated grapheme_break_property_data_gen.py accordingly (and verified that the output is identical after clang-formatting).
    • There's a newline embedded at the beginning of our multi-line "TEMPLATE" raw string literals. I took @JMazurkiewicz's suggestion of calling PROP_VALUE_ENUM_TEMPLATE.lstrip() which is sufficient. But then I noticed that DATA_ARRAY_TEMPLATE was the only multi-line string template that we weren't stripping leading whitespace from, which seemed confusing. I think it's better to consistently strip them (so we can pretend that they didn't begin with leading whitespace at all), and then explicitly add a newline to separate the enumerator values from the table.
  • Change comments to refer to _Count instead of _Count_raw.
    • Mentioning _Count is overwhelmingly more common; _Uninitialized_value_construct_n_unchecked1 in <xmemory> doesn't even take _Count_raw.
  • Fix a comment typo: "Ad hoc" is two words.

@StephanTLavavej StephanTLavavej added the documentation Related to documentation or comments label Mar 3, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 3, 2023 19:17
@github-actions github-actions bot added this to Initial Review in Code Reviews Mar 3, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Work In Progress in Code Reviews Mar 3, 2023
@StephanTLavavej StephanTLavavej self-assigned this Mar 3, 2023
Co-authored-by: Jakub Mazurkiewicz <mazkuba3@gmail.com>
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Final Review in Code Reviews Mar 3, 2023
@StephanTLavavej StephanTLavavej removed their assignment Mar 3, 2023
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

I've noticed two areas where we may want further changes:

  • stl/inc/iterator:255: it doesn't result in the newline going away, but we could change the // strengthened to a /* strengthened */ before a { (I believe it's the only one left
  • there are still two newlines in a row between macros in <atomic>

@strega-nil-ms strega-nil-ms moved this from Final Review to Ready To Merge in Code Reviews Mar 6, 2023
@StephanTLavavej
Copy link
Member Author

Thanks @strega-nil-ms! 😻 I've pushed a conflict-free merge with main, followed by commits for the 2 issues you mentioned. I had originally skipped that <iterator> line because it was too long, but you're right that it's simpler to just change it and get rid of the special case. And great catch for <atomic> - I think I had excluded it because #3406 was in flight, but then I forgot about it.

@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 326473d into microsoft:main Mar 7, 2023
Code Reviews automation moved this from Ready To Merge to Done Mar 7, 2023
@StephanTLavavej StephanTLavavej deleted the stl-cleanups-formatting-comments branch March 7, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Related to documentation or comments
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants