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

Update preprocessor #else comment in product code to clarify which mode is where #3900

Merged
merged 38 commits into from
Aug 10, 2023

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Jul 22, 2023

Towards #351. Product headers.

  • Product files, all found places except a few I'm not sure about (plan to do a separate subsequent change).
  • No /tests yet (but there are fewer places)
  • Changed comments like #else // _HAS_EXCEPTIONS to #else // ^^^ _HAS_EXCEPTIONS / !_HAS_EXCEPTIONS vvv
  • In some of one line in each branch cases removed comment altogether
  • Fixed some of pre-existing ^^^ / vvv comments, mostly like !_M_CEE -> !defined(_M_CEE)
  • Fixed some #endif coments to #endif // defined(...) / #endif // !defined(...)

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner July 22, 2023 08:14
@AlexGuteniev AlexGuteniev changed the title Update most of the preprocessor #else comment in product headers to clarify which mode is where Update most of the preprocessor #else comment in product code to clarify which mode is where Jul 22, 2023
@AlexGuteniev AlexGuteniev changed the title Update most of the preprocessor #else comment in product code to clarify which mode is where Update preprocessor #else comment in product code to clarify which mode is where Jul 22, 2023
@CaseyCarter CaseyCarter added enhancement Something can be improved documentation Related to documentation or comments and removed enhancement Something can be improved labels Jul 22, 2023
@StephanTLavavej StephanTLavavej self-assigned this Jul 22, 2023
stl/inc/__msvc_bit_utils.hpp Outdated Show resolved Hide resolved
stl/inc/__msvc_bit_utils.hpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I pushed a bunch of commits since it was just easier to fix stuff than do my usual process of adding a GitHub comment followed by a paired commit. Tried to keep them organized:

  • Pre-existing: not _USE_STD_VECTOR_ALGORITHMS => !_USE_STD_VECTOR_ALGORITHMS
  • Pre-existing: Simplify _HAS_CXX20 comments in <coroutine>.
  • Pre-existing: ^^^ / vvv => /
  • ^^^ / vvv => /
  • Fix !!defined(__AVX2__) typos.
  • Fix _CPPUNWIND arrow comment.
  • Extra space: // defined(_CPPUNWIND) => // defined(_CPPUNWIND)
  • Nested preprocessor logic should always be commented.
  • Update #ifdef __cpp_lib_concepts comments (mostly pre-existing).
  • Fix _CMPXCHG_MASK_OUT_PADDING_BITS arrow comment.
  • Consistent arrow comments.
  • Add missing spaces.
  • After #ifndef, comment !defined.
  • Typo: ___FORCE_INSTANCE had 3 underscores.
  • Drop an unnecessary empty line.
  • Fix arrow comment.
  • Within _HAS_CXX20, note that we have a workaround for lacking concepts.
  • Fix comments that should have said defined; use the shorter warning disabled / warning enabled form.
  • Non-comment style changes to align with comments: Always use parentheses with defined.

@StephanTLavavej StephanTLavavej removed their assignment Jul 24, 2023
@AlexGuteniev
Copy link
Contributor Author

I see da43a31 restored a couple of #else // _M_CEE.
Will not push a commit to address this, as there's already one approve, and I was planning a subsequent change for few remaining occurrences anyway.

@StephanTLavavej
Copy link
Member

The principle is that in nested preprocessor code, we shouldn't omit comments. (The only exception are in the feature-test macro test, and the SCL/HID defaults, where they would lead to unproductive clutter.) Arrow comments would be fine, but omitting the inner layer of comments is contrary to our current convention, even though these are pretty simple cases.

@StephanTLavavej
Copy link
Member

I've pushed a merge with main to resolve a trivial merge conflict in yvals_core.h; this PR edited an #endif comment next to the addition of __cpp_lib_formatters.

@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 9ad382e into microsoft:main Aug 10, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

#endif // ^^^ THANKS! ^^^

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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants