Skip to content

Update zc-conformance.md for accuracy and consistency #4665

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

Closed
wants to merge 1 commit into from

Conversation

balagansky-work
Copy link

Changes assume accuracy of the individual option doc pages

  • Update default values per individual option docs
  • Consistent implied by wording and formatting
  • Mention [-] syntax where applicable

@learn-build-service-prod
Copy link
Contributor

Learn Build status updates of commit d3fafb0:

✅ Validation status: passed

File Status Preview URL Details
docs/build/reference/zc-conformance.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Court72
Copy link
Contributor

Court72 commented Aug 1, 2023

@balagansky-work

Thank you for your contribution. Would you take a moment to sign the Contributor License Agreement (CLA)? After the CLA is signed, someone can review your pull request. Thanks!

#label:"aq-pr-triaged"
@MicrosoftDocs/public-repo-pr-review-team

@prmerger-automator prmerger-automator bot added the aq-pr-triaged Tracking label for the PR review team label Aug 1, 2023
@balagansky-work
Copy link
Author

@balagansky-work

Thank you for your contribution. Would you take a moment to sign the Contributor License Agreement (CLA)? After the CLA is signed, someone can review your pull request. Thanks!

#label:"aq-pr-triaged" @MicrosoftDocs/public-repo-pr-review-team

Apologies for the delay. I'm still waiting on my company's approval to sign the CLA.

| [`/Zc:char8_t`](zc-char8-t.md) | Enable or disable C++20 native `u8` literal support as `const char8_t` (off by default, except under **`/std:c++20`**). |
| [`/Zc:alignedNew[-]`](zc-alignednew.md) | Enable C++17 over-aligned dynamic allocation (implied by **`/std:c++17`** or later). |
| [`/Zc:auto[-]`](zc-auto-deduce-variable-type.md) | Enforce the new Standard C++ meaning for **`auto`** (on by default). |
| [`/Zc:char8_t[-]`](zc-char8-t.md) | Enable or disable C++20 native `u8` literal support as `const char8_t` (implied by **`/std:c++20`** or later). |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi, I sometimes like the 'implied by' approach, but it doesn't mean the same thing as the text it replaces. It indicates that the switch may be on when another switch is specified, but it loses the clarity that it is off in all other cases. It replaces a statement that is true in all cases with a more ambiguous one that it is true in a specific case. I prefer the wording for /Zc:char8_t, for example. I'd rather not take the 'implied by' changes unless they add the extra clarity that they are off, otherwise. Thank you for adding the optional [-] notes!

Copy link
Author

Choose a reason for hiding this comment

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

My goal was to make these parentheticals consistent, which improves readability. I chose the "implied by" form because it was terser. But I don't mind changing it over to the form on this line.

Some cases are a bit different than others. If you still don't like the wording, let me know and I'll update it.

Copy link
Author

Choose a reason for hiding this comment

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

I replaced "implied by" everywhere with the form on line 29.

I also made another tweak to the twoPhase row. That one is a trickier special case. I hope the new wording is clearer. I think the original wording was prone to misinterpretation.

@learn-build-service-prod
Copy link
Contributor

Learn Build status updates of commit cc3c5f8:

✅ Validation status: passed

File Status Preview URL Details
docs/build/reference/zc-conformance.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Changes assume accuracy of the individual option doc pages

* Update default values per individual option docs
* Use consistent wording and formatting for default values
* Mention [-] syntax where applicable
@learn-build-service-prod
Copy link
Contributor

Learn Build status updates of commit db05a2d:

✅ Validation status: passed

File Status Preview URL Details
docs/build/reference/zc-conformance.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Collaborator

@TylerMSFT TylerMSFT left a comment

Choose a reason for hiding this comment

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

I like what you did there. Thank you!

@TylerMSFT
Copy link
Collaborator

#sign-off

@Court72
Copy link
Contributor

Court72 commented Aug 18, 2023

Apologies for the delay. I'm still waiting on my company's approval to sign the CLA.

@balagansky-work - No problem! After you've signed the CLA, this PR can be merged. Thanks!

#hold-off

@balagansky-work
Copy link
Author

Unfortunately, my company's process for signing this CLA is time consuming and costly relative to the trivial nature of this change.

I notice in https://github.com/MicrosoftDocs/cpp-docs/blob/main/CONTRIBUTING.md the possibility of waiving the CLA signature requirement for minor changes.

Would this PR perhaps be a candidate for waiving the CLA requirement? If not, I will need to close the PR.

Thank you and apologies for the inconvenience.

@Court72
Copy link
Contributor

Court72 commented Aug 29, 2023

@balagansky-work - Thanks for letting us know. The CLA completion is required for this PR.

@TylerMSFT - Would you like to pull these changes into the private repo and merge?

@orcmid
Copy link

orcmid commented Aug 29, 2023

Side Question: Is there some threshold that triggers the CLA requirement on a pull request? Or does it happen on any pull request for someone without a CLA on record? (I have one already so I can't tell if there is any difference.)

@Court72 your comments seem contradictory. Is that how a decision that a change is minor works?

@Court72
Copy link
Contributor

Court72 commented Aug 29, 2023

@orcmid - That's correct! From the documentation @balagansky-work linked: "When your PR is created, it is classified by a CLA bot. If the change is trivial (for example, you fixed a typo), then the PR is labeled with CLA-not-required. Otherwise, it's classified as CLA-required. Once you signed the CLA, the current and all future pull requests are labeled as CLA-signed."

@TylerMSFT
Copy link
Collaborator

I don't want to lose the work that @balagansky-work has put in here. I'll port the changes and check in under my account.
@balagansky-work, I'll let you know once that's done.

@TylerMSFT
Copy link
Collaborator

@balagansky-work, I have taken your changes and put them in a private repo and integrated them into the docs. They'll be live by tomorrow.
The PR is https://github.com/MicrosoftDocs/cpp-docs-pr/pull/5022
Thank you for contributing to the docs!

@TylerMSFT TylerMSFT closed this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aq-pr-triaged Tracking label for the PR review team do-not-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants