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

[conv.lval] Improve the note about std::nullptr_t case #2372

Merged
merged 2 commits into from Nov 26, 2018

Conversation

k-satoda
Copy link
Contributor

The meaning of "fetched from memory" is vague, and the two uses of term
"access" [defns.access] are inappropreate here because

  • it causes undefined behavior to access an inactive member of a
    union, based on the rule of out-of-lifetime object [basic.life]:

    The program has undefined behavior if:

    • the glvalue is used to access the object, or
  • it implies reading the referenced object of std::nullptr_t,
    which is defined to be a side effect for volatile-qualified case.
    [intro.execution]

    Reading an object designated by a volatile glvalue, ... are all
    side effects, ...


The change blindly assumes that "no value is fetched from memory" is
true and rephrases it as "the conversion does not read the object", but
I'm not sure how it is true. In fact, gcc 8.2 emits read instruction for
volatile-qualified case https://godbolt.org/z/uhn72l (clang 7.0.0
doesn't), and both gcc 8.2 and clang 7.0.0 reads a member subobject of
type std::nullptr_t https://godbolt.org/z/fqBU-H.

FYI: The note was added by CWG issue 2140 for C++17.
https://wg21.cmeerw.net/cwg/issue2140
commit 28c12a8

The meaning of "fetched from memory" is vague, and the two uses of term
"access" [defns.access] are inappropreate here because
  - it causes undefined behavior to access an inactive member of a
    union, based on the rule of out-of-lifetime object [basic.life]:
    > The program has undefined behavior if:
    >   - the glvalue is used to access the object, or
  - it implies reading the referenced object of std::nullptr_t,
    which is defined to be a side effect for volatile-qualified case.
    [intro.execution]
    > Reading an object designated by a volatile glvalue, ... are all
    > side effects, ...
Since no value is fetched from memory,
there is no side effect for a volatile access\iref{intro.execution}, and
an inactive member of a union\iref{class.union} may be accessed.
Since the conversion does not read the object,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "read" -> "access"?
Do we know what "the object" refers to, here? I'm not seeing an antecedent.

Copy link
Contributor Author

@k-satoda k-satoda Oct 30, 2018

Choose a reason for hiding this comment

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

I'm ok with "read" -> "access".

For "the object", I found 3 phrasing to say the same thing:

  • "the referenced object" in the previous paragraph.
  • "the object to which the glvalue refers" at the bullet of invalid pointer value.
  • "the object indicated by the glvalue" at the last bullet.

I choose the second as the most clear one.

Added commit 45edb77.

- "read the object" -> "access the object"
  The "read" was derived from the rule at [intro.execution], but it
  didn't fit well here.
- "the object" -> "the object to which the glvalue refers"
  Make it clear what "the object" refers to.

cplusplus#2372 (review)
@k-satoda
Copy link
Contributor Author

k-satoda commented Oct 30, 2018

I found a bug report against clang about access on inactive
std::nullptr_t member of a union.
"If the contents of a nullptr_t are modified through type-punned union, conversions from that nullptr_t to bool don't behave the same as conversion from nullptr to bool"
https://bugs.llvm.org/show_bug.cgi?id=23833
The test case still fails with clang 7.0.0 which means an exisiting
disagreement between the note and an implementation. Hmm.
https://wandbox.org/permlink/8UWvvFWLC1pF0OyK

@zygoloid
Copy link
Member

The test case still fails with clang 7.0.0 which means an exisiting
disagreement between the note and an implementation. Hmm.

Implementations take time to implement DR resolutions. (FWIW, I just fixed this in Clang trunk.)

@k-satoda
Copy link
Contributor Author

Understood. Glad to see the fix.

As for how can we say "no value is fetched from memory" or
"the conversion does not access ..." from non-note rules, my best guess
so far is that the last bullet ("the value contained in the object
indicated by the glvalue is the prvalue result") determines an access as
it implies reading the value. That means the same exemption can be
applied for empty classes;

struct S {};
union { int x; S y; } a = {37};
S s = a.y; // No undefined behavior?

because member-wise copy (defined at [class.copy.ctor]) for such a class
does no further lvalue-to-rvalue conversions.
Correct?

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

This seems to be a (slight) net improvement of the wording in a note. Fine with me.

@zygoloid zygoloid merged commit 8ddcba4 into cplusplus:master Nov 26, 2018
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.

None yet

3 participants