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 cppguide #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zachfang
Copy link

@zachfang zachfang commented Dec 20, 2024

Relate to RobotLocomotion/drake#22409

This change is Reviewable

Copy link
Author

@zachfang zachfang left a comment

Choose a reason for hiding this comment

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

+@jwnimmer-tri for review as discussed in a Slack thread.
The second commit is an extra thing I crossed out but can be dropped if it doesn't makes sense contextually.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

Copy link

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: feature.

For meaningful styleguide changes, we always tag the full platform team for review. However, I'd say these are just wordsmithing typos to adjust the guide to our already-evident reality, so don't need the full team.

So, for next steps, please open a Drake (draft) PR that bumps to this sha, so that we can preview the changes. Then, assign the platform reviewer on-call to platform review this PR.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @zachfang)

Copy link
Author

@zachfang zachfang left a comment

Choose a reason for hiding this comment

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

+@ggould-tri for platform review, thanks!

Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri)

Copy link
Collaborator

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: as to the filesystem change.
I believe the exception change is potentially too controversial to evade whole-team review and and have commented it as such.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @zachfang)


cppguide.html line 1328 at r1 (raw file):

  <li>There is no easy way for constructors to signal errors, short of
  crashing the program (not always appropriate) or using exceptions
  <code class="nondrake">(which are <a href="#Exceptions">forbidden</a>)</code>.</li>

This is a potentially controversial change: Throwing an exception in a constructor is a matter of dispute, in part because it prevents the corresponding destructor from running; a nontrivial destructor (for instance one that manages global resources such as file handles or network sockets) would make an exceptionful constructor unsound.

(This is even more true if this ctor is called from another nontrivial ctor, as the caller will have some but not all of its members destroyed, and this member freed without destruction, which the author may not have planned for. See https://stackoverflow.com/questions/10212842/what-destructors-are-run-when-the-constructor-throws-an-exception and the Herb Sutter post it references for the reason that this is usually reasonable.)

So while "exceptions are forbidden" is no longer true, this paragraph is in the specific context of ctors where exceptions are at least something that would require broader discussion.

I don't think it belongs in the same PR as the extremely uncontroversial change below.

Copy link

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @ggould-tri)


cppguide.html line 1328 at r1 (raw file):

Previously, ggould-tri wrote…

This is a potentially controversial change: Throwing an exception in a constructor is a matter of dispute, in part because it prevents the corresponding destructor from running; a nontrivial destructor (for instance one that manages global resources such as file handles or network sockets) would make an exceptionful constructor unsound.

(This is even more true if this ctor is called from another nontrivial ctor, as the caller will have some but not all of its members destroyed, and this member freed without destruction, which the author may not have planned for. See https://stackoverflow.com/questions/10212842/what-destructors-are-run-when-the-constructor-throws-an-exception and the Herb Sutter post it references for the reason that this is usually reasonable.)

So while "exceptions are forbidden" is no longer true, this paragraph is in the specific context of ctors where exceptions are at least something that would require broader discussion.

I don't think it belongs in the same PR as the extremely uncontroversial change below.

More PRs is just hassle with no upside. Let's just keep everything in one place here.

I suppose the next step is to clarify the wording here beyond just the strikethrough. We throw from our constructor all the time (e.g., DRAKE_THROW_UNLESS to validate the ctor arguments) so it's important to not forbid it here. What's missing is a bit more text explaining the caveats -- the dtor will not run, so e.g. be sure to do all your throw-checking up front before starting into resource-claiming actions especially ones with out proper RAII.

@zachfang zachfang force-pushed the feature/remove-filesystem branch from b914285 to 2b4fe11 Compare January 9, 2025 07:13
Copy link
Author

@zachfang zachfang left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion (waiting on @ggould-tri and @jwnimmer-tri)


cppguide.html line 1328 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

More PRs is just hassle with no upside. Let's just keep everything in one place here.

I suppose the next step is to clarify the wording here beyond just the strikethrough. We throw from our constructor all the time (e.g., DRAKE_THROW_UNLESS to validate the ctor arguments) so it's important to not forbid it here. What's missing is a bit more text explaining the caveats -- the dtor will not run, so e.g. be sure to do all your throw-checking up front before starting into resource-claiming actions especially ones with out proper RAII.

I have added a note right after the strikethrough. Let me know if the wording needs tweaking. The rendered HTML looked good to me in terms of formatting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants