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

Use Brace Lists to initialize objects. #3277

Merged
merged 10 commits into from
Feb 3, 2023
Merged

Conversation

RSilicon
Copy link
Contributor

@RSilicon RSilicon commented Dec 9, 2022

This is for consistency with the rest of the code.

This is for consistency.
@RSilicon RSilicon requested a review from a team as a code owner December 9, 2022 17:28
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Dec 10, 2022
@StephanTLavavej
Copy link
Member

Since we're constructing temporaries, I would prefer to see path{ARGS}, pos_type{ARGS}, and locale{ARGS}, instead of just {ARGS}. This is a case where I feel that writing a bit more than the compiler demands is justified, because it highlights the construction of the temporary.

In contrast, plain return { STUFF }; is reasonable when the return type is pair/tuple-like, because the construction of the temporary is not especially interesting or expensive. (For example, range algorithms have return types like this.)

@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2023
stl/inc/strstream Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I've pushed a merge with main to resolve a simple conflict with a comment change, followed by commits to address code review feedback (as I mentioned, we think that mentioning the types would be a good idea here) and update more occurrences of these types. This significantly expands the size of the diff, but it is still primarily focused on path, locale, and pos_type (plus a couple of things in their immediate vicinity); I tried not to make this PR excessively sprawling, and instead simply tried to complete its improvements for consistency.

To summarize, the changes are:

  • Update all occurrences of path, locale, and pos_type to use braces when constructing temporaries (or allocating objects).
  • Drop unnecessary parentheses in two successive lines that were constructing paths.
  • Also update file_status and off_type to use braces. (The latter is ultimately long long, so off_type(-1) was a functional-style cast from int to long long.)
  • We were inconsistent about whether to construct pos_type (i.e. fpos) directly from -1, or from an off_type. I've chosen to standardize on the full pos_type{off_type{-1}} form, as this more closely aligns with what the Standard depicts (and ultimately there is no codegen impact).

Note: I have deliberately not audited the test code. Our test suite is relatively vast and contains lots of old code. We don't spend effort on modernizing its patterns except in unusually important cases (e.g. braces for control flow); also it is sometimes important for test code to use unusual patterns so by default it's better not to mess with them too much. We try to keep the product code more modern and consistent.

@StephanTLavavej StephanTLavavej removed their assignment Feb 1, 2023
@CaseyCarter CaseyCarter self-assigned this Feb 1, 2023
@StephanTLavavej StephanTLavavej self-assigned this Feb 2, 2023
@StephanTLavavej
Copy link
Member

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

@RSilicon RSilicon changed the title Use Brace List to initialize objects. Use Brace Lists to initialize objects. Feb 2, 2023
@StephanTLavavej StephanTLavavej merged commit 73924c1 into microsoft:main Feb 3, 2023
@StephanTLavavej
Copy link
Member

Thanks for these consistency improvements! 📈 😸 🎉

@RSilicon RSilicon deleted the brace branch February 3, 2023 00:59
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
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
  翻译: