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

[class.dtor] Change redundant wording into note #3966

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

Conversation

sdkrystian
Copy link
Contributor

First sentence was recommended by @jensmaurer (non-pure virtual functions are always odr-used), the second is redundant because destructors are non-static data members and inheritance doesn't play into whether a virtual function is overridden (i.e. this is covered by [class.virtual] p1 & p2).

source/classes.tex Outdated Show resolved Hide resolved
@sdkrystian sdkrystian force-pushed the patch-76 branch 2 times, most recently from ecc432a to ed0bc33 Compare May 2, 2020 21:24
Comment on lines 2267 to 2104
\begin{note}
If the destructor of a class is virtual and
any objects of that class or any derived class are created in the program,
the destructor shall be defined.
If a class has a base class with a virtual destructor, its destructor
(whether user- or implicitly-declared) is virtual.
the destructor must be defined; see~\ref{basic.def.odr}.
\end{note}
\begin{note}
If a class \tcode{C} has a base class with a virtual destructor,
the destructor of \tcode{C} (whether user- or implicitly-declared) is virtual\iref{class.virtual}.
\end{note}
Copy link
Member

@zygoloid zygoloid Oct 18, 2020

Choose a reason for hiding this comment

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

This seems wrong, both before and after these changes. If the destructor is virtual and not pure virtual, it must be defined regardless of whether any objects exist. I think the intent is to talk about pure virtual destructors here, in which case "any objects of that class or any derived class are created" is not the right constraint either! Merely deriving from the class with a class that has a non-pure virtual destructor is enough to (indirectly) require the pure virtual base class destructor to exist, because the derived class destructor is virtual and therefore odr-used, and it in turn odr-uses the base class destructor.

So I think instead we should say something like:

[Note: A pure virtual destructor must still be defined in most cases, because it is invoked by the destructor of any derived class.]

@zygoloid zygoloid added changes requested Changes to the wording or approach have been requested and not yet applied. needs rebase The pull request needs a git rebase to resolve merge conflicts. labels Oct 18, 2020
@jensmaurer jensmaurer removed this from the C++23 milestone Oct 7, 2023
the destructor shall be defined.
\begin{note}
Since a pure virtual destructor will be invoked by the destructor of
a derived class, it must still be defined in most cases; see~\ref{basic.def.odr}.
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use "must" in notes, and certainly not in this way: we're not talking about an existing, external requirement here, but we're explaining the reason for the rule in [basic.def.odr].

@jensmaurer, @opensdh, any ideas for how to phrase an "explanation of a rule"? How about:

"Since a pure virtual destructor will be invoked by the destructor of a derived class, it has a definition in most cases; see [basic.def.odr]."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Since a pure virtual destructor will be invoked by the destructor of a derived class, a definition is generally required; see [basic.def.odr]."

Another alternative would be "usually required".

Copy link
Contributor

Choose a reason for hiding this comment

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

@zygoloid suggested:

"Because a pure virtual destructor will be invoked by the destructor of a derived class, [basic.def.odr] requires a definition in most cases."

I'm a bit hesitant about having "requirements in notes", which we aren't allowed to, and maybe this latter form is a bit further from the forms ISO considers "requirements" than "is required" (https://www.iso.org/sites/directives/current/part2/index.xhtml#_idTextAnchor082).

Copy link
Contributor

@opensdh opensdh Nov 9, 2023

Choose a reason for hiding this comment

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

How about

[Note: Even a pure virtual destructor is odr-used by any derived concrete class. — end note]

to avoid talking about any requirement at all? Otherwise, you can use statements like "… is necessary" that more clearly appear to be (mere) statements of fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's quite correct, since an abstract derived class will call the destructor of a direct non-virtual abstract base class. I'm also not a fan of starting the sentence with "Even", as it implies elaboration on a previous broader statement. How about:

[Note: The destructor of a derived class will odr-use ([basic.def.odr]) the selected destructor of a direct base class -- even if that destructor is pure virtual. -- end note]

Or

[Note: A pure virtual selected destructor will be odr-used ([basic.def.odr]) by any derived classes. -- end note]

Copy link
Contributor

@opensdh opensdh Nov 10, 2023

Choose a reason for hiding this comment

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

The note doesn't have to be exhaustive; having a concrete class is a sufficient but not necessary condition for the odr-use. If you like it better, though, the first version you gave seems correct with minor edits:

[Note: A definition of a destructor of a derived class odr-uses ([basic.def.odr]) the selected destructor of a direct base class, even if that destructor is pure virtual. — end note]

I don't think the second is right, since those derived classes could all have pure virtual destructors with no definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second version is intended to be more concise but less precise... I'll apply your suggested changes to the first version next week :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes to the wording or approach have been requested and not yet applied. needs rebase The pull request needs a git rebase to resolve merge conflicts.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants