cplusplus / CWG

Core Working Group
23 stars 7 forks source link

CWG2781 [basic.def.odr] "... for D itself, the behavior is as if there is a single entity with a single definition" renders the complere ODR teethless #402

Open RealLitb opened 1 year ago

RealLitb commented 1 year ago

The ODR applies to multiple definitions of the same entity. For example this is rendered IFNDR by https://eel.is/c++draft/basic.def.odr#14.5 :

In each such definition, corresponding names, looked up according to [basic.lookup], shall refer to the same entity, ...

int x;
inline int f() { return x; } // refers to different entities!

But there is a rule in p15:

For each such entity and for D itself, the behavior is as if there is a single entity with a single definition, including in the application of these requirements to other entities.

Therefore, the above code example is not anymore IFNDR because the behave is "as if" there is only a single definition of f? I hear that this is not the case, because the requirements still apply. So if we change f to this:

inline int f() { return []{return 0;}(); } 

If we check the requirements in p14 and then for the remainder of the spec behave as if there was only a single definition of D (which is what prior specs said), then the requirement in p14 that says this is violated and the function f becomes IFNDR:

In each such definition, except within the default arguments and default template arguments of D, corresponding lambda-expressions shall have the same closure type

t3nsor commented 1 year ago

Possible duplicate of https://github.com/cplusplus/CWG/issues/271

RealLitb commented 1 year ago

Unfortunately #271 is closed and the wording is still highly ambiguous. I see no way to read p14 and p15 to make the first example IFNDR and the second example wellformed.

t3nsor commented 1 year ago

If enough people want #271 fixed, maybe it will get reopened :)

jensmaurer commented 1 year ago

A clarification question:

For your initial example, I'm missing a bit of context:

int x;
inline int f() { return x; } // refers to different entities!

Is this supposed to appear in several translation units of a program? x (alone) has external linkage and is a definition here, so this seems to be IFNDR right there (due to multiple definitions for x). Or did you mean to put static in front of int x?

Also, you seem to be referring a rule in p15 that is now in p16:

These requirements also apply to corresponding entities defined within each definition of D (including the closure types of lambda-expressions, but excluding entities defined within default arguments or default template arguments of either D or an entity not defined within D). For each such entity and for D itself, the behavior is...

I feel "For each such entity" clearly refers to "corresponding entities defined within each definition of D".

Are we just missing something like "For each such entity and for D itself where these requirements are satisfied, the behavior is..." ?

t3nsor commented 1 year ago

I think that was the thrust of @RealLitb 's concern with the wording, but then we run into the issue from #271. It would imply that you check all the conditions in p14 first before you can assume any of the guarantees provided by p15, which is not the intent.

RealLitb commented 1 year ago

A clarification question:

For your initial example, I'm missing a bit of context:

int x;
inline int f() { return x; } // refers to different entities!

Is this supposed to appear in several translation units of a program? x (alone) has external linkage and is a definition here, so this seems to be IFNDR right there (due to multiple definitions for x). Or did you mean to put static in front of int x?

Yes, sorry. I meant to put "static" in front of "int x". The code is meant to appear in multiple TUs.

Also, you seem to be referring a rule in p15 that is now in p16:

These requirements also apply to corresponding entities defined within each definition of D (including the closure types of lambda-expressions, but excluding entities defined within default arguments or default template arguments of either D or an entity not defined within D). For each such entity and for D itself, the behavior is...

I feel "For each such entity" clearly refers to "corresponding entities defined within each definition of D".

Are we just missing something like "For each such entity and for D itself where these requirements are satisfied, the behavior is..." ?

I think this issue report is a dupe of #271 (which I definitely should have read more closely before opening mine). Even if you add the text "where these requirements are satisfied", then it means that for cases like in #271 or in cases like in my lambda example (where there is a local declaration of a struct - that albeit is made to have the token sequence of the lambda expression for ODR purposes, still declares a new local type - that would presumably, because it has no linkage, then declare a different entity in each of the TUs that the inline definition appears in).

There is a note in [dcl.inline]p6 (https://eel.is/c++draft/dcl.inline#6) that says

[Note 3: An inline function or variable with external or module linkage can be defined in multiple translation units ([basic.def.odr]), but is one entity with one address. A type or static variable defined in the body of such a function is therefore a single entity. — end note]

However, the normative text that guarantees this appears to rely entirely on the "for D itself, the behavior is as if there is a single entity with a single definition" part of the ODR. So it's important that we first apply that rule, to then satisfy the requirement in p14 that names within the inline definition that happen to refer to entities defined therein refer to the same entity. Inserting "where these requirements are satisfied" reverses the order, however, and therefore makes the lambda example and the example in #271 fail with IFNDR.

RealLitb commented 1 year ago

I think I see now what you are saying, which I was missing at first. I think it works if we only apply p15 to D if the requirements are satisfied, but apply them to "each such entity" regardless of whether they are satisfied (in the check of D, but not in the (recursive) check for each of the entities. So in the recursive checks, all definitions are checked again.

So I think I would recommend:

If the requirements are satisfied, the behavior for D is as if there is a single entity with a single definition, including in the application of these requirements to other entities. This behavior also applies to each corresponding entity in the above requirements on definitions of D.

Maybe with a clarifying note in the comments.

jensmaurer commented 1 year ago

First of all, let's please use the latest working draft https://isocpp.org/files/papers/N4958.pdf for any specific discussions. I believe it contains minor editorial improvements that may or may not be helpful for this discussion. At least, we should use its paragraph number for further discussion.

Furthmore, let's consider #271 dead (even though this issue might be a duplicate), and make sure we reach a common understanding here.

Please bear with me while I walk through the examples that have been presented here, and please tell me where I take the wrong turn according to your reading of the wording. All references are to [basic.def.odr].

static int x;
inline int f() { return x; } // refers to different entities!

Here, we want to find out whether the definition of f is IFNDR, which is our "D".

p14.2 tells use we need to check "the following requirements". p14.5 says we need to check the lookup results for x. Those refer to distinct entities in different translation units, so the requirements are not met. The rule "For each such entity and for D itself, the behavior is as if there is a single entity" doesn't change anything, because x is not defined within D. However, it does make f have a single definition for purposes of the ODR for any other checking we need to do elsewhere.

Now, on to the case in #271, slightly simplified:

inline void f2(){
   int a = 0;
   int b = a;
}

We again arrive at p14.5 for D = f2. We have to check whether a refers to the same entity in both definitions. At first sight, it doesn't, but let's read on.

p16 says "These requirements also apply to corresponding entities defined within each definition of D". So, the requirements apply (recursively) to a, for which they are satisfied. In the invocation of the rules for D = a, we arrive at " ... for D itself, the behavior is as if there is a single entity with a single definition, including in the application of these requirements to other entities."

So, we consider "a" to have a single definition, and we use that knowledge when resuming the checking for D = f2, where we also reach the "single definition" conclusion.

I do agree that a reference to the recursive requirements checking on a does not clearly imply that we do the "as-if single definition" thing at every recursion step.

jensmaurer commented 1 year ago

https://cplusplus.github.io/CWG/issues/2781.html

xmh0511 commented 1 year ago

Why do we not prefer to rearrange a similar assumption to the proposed wording

This behavior also apply applies to corresponding entities defined within each definition of D (including the closure types of lambda-expressions, but excluding entities defined within default arguments or default template arguments of either D or an entity not defined within D)

before the requirement list that is used to check D? That is before we check some D, we first have an assumption that all entities that are defined or be considered defined within the definition of D are assumed as if they have the same single definition such that we can check whether D violate or obey ODR-rule, this is natural like we invoke a program for checking.