Skip to content

Use Brace Lists to initialize objects.#3277

Merged
StephanTLavavej merged 10 commits into
microsoft:mainfrom
mike-goutokuji:brace
Feb 3, 2023
Merged

Use Brace Lists to initialize objects.#3277
StephanTLavavej merged 10 commits into
microsoft:mainfrom
mike-goutokuji:brace

Conversation

@mike-goutokuji
Copy link
Copy Markdown
Contributor

This is for consistency with the rest of the code.

This is for consistency.
@mike-goutokuji mike-goutokuji 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
Copy Markdown
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
Comment thread stl/inc/strstream Outdated
@StephanTLavavej
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@mike-goutokuji mike-goutokuji 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
Copy Markdown
Member

Thanks for these consistency improvements! 📈 😸 🎉

@mike-goutokuji mike-goutokuji 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