Page MenuHomePhabricator

[clang] Also consider structured bindings when checking whether a local entity is odr-used in a default argument.
Needs ReviewPublic

Authored by riccibruno on Aug 9 2020, 9:25 AM.

Details

Summary

Currently clang accepts a default argument containing an odr-used structured binding which is not odr-usable here:

void test() {
  struct S { int i, j; };
  auto [x, y] = S();
  extern void h(int = x); // Should be rejected.
}

It was not entirely clear in C++17 that this was forbidden since [dcl.fct.default]p7 only used the term "local variable". However this is clearly forbidden in C++20 with the new wording in term of odr-usable local entities.

Diff Detail

Event Timeline

riccibruno created this revision.Aug 9 2020, 9:25 AM
riccibruno requested review of this revision.Aug 9 2020, 9:25 AM
riccibruno edited the summary of this revision. (Show Details)Aug 9 2020, 2:11 PM
rsmith added inline comments.Aug 9 2020, 11:09 PM
clang/lib/Sema/SemaDeclCXX.cpp
101–105

Is there a reason to separate these two cases? (Could we just use the decomposed decl unconditionally?) Generally treating tuple-like decompositions differently from other kinds seems error-prone.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
44

The diagnostic in this case is inconsistent with the non-tuple-like cases. I think this diagnostic is better; we should use the original Decl when producing the diagnostic, not the decomposed variable.

riccibruno marked 2 inline comments as done.
riccibruno edited the summary of this revision. (Show Details)

Refer to the binding in the diagnostic.

riccibruno added inline comments.Aug 10 2020, 7:52 AM
clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
44

Right, let's just refer to the binding in all cases.

rsmith added inline comments.Aug 10 2020, 9:36 AM
clang/include/clang/AST/DeclCXX.h
3843 ↗(On Diff #284364)

!! This seems pretty bad; would it be hard to fix?

riccibruno added inline comments.
clang/include/clang/AST/DeclCXX.h
3843 ↗(On Diff #284364)

On further examination, I believe that`Decomp` is set but this is subtle, and it is likely that I am missing a case/wrong somehow.

The expression for the binding (Binding) will be deserialized when visiting the BindingDecl. This expression when non-null will always (as far as I can tell) contain a reference to the decomposition declaration so the decomposition will be deserialized, which will set Decomp.

The binding expression is null when the initializer is type-dependent. But then, since variable template decompositions are not allowed, the decomposition must occurs at block scope (mayHaveDecompositionDeclarator). This means that the DecompositionDecl will be read first.

riccibruno added inline comments.Aug 11 2020, 7:41 AM
clang/lib/Sema/SemaDeclCXX.cpp
112

This can use DiagnoseIfOdrUse as soon as the inconsistency between parameters and local variables is removed.

Remove the now-unused const VarDecl * parameter to DiagnoseIfOdrUse.

riccibruno marked an inline comment as done.Aug 21 2020, 9:01 AM
aaronpuchert added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
91

Since you're emitting a specific warning, I think you can just hardcode the expected types here instead of using const auto&.... Alternatively you could guess the integer argument from the (dynamic) type of the VarDecl argument.

145

Have you seen a case there the _or_null is relevant? To me it seems like this shouldn't happen, at least not when we get here.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
40

Better use a relative line number: [[@LINE-5]] or something like that.

riccibruno marked 2 inline comments as done.Sep 2 2020, 10:17 AM
riccibruno added inline comments.
clang/lib/Sema/SemaDeclCXX.cpp
91

I am using a pack here (changed a bit in the latest revision) to make it easy to re-use this lambda when the inconsistency between parameters and local variables is removed.

145

I have removed it in the latest revision since we should fix the (de)serialization instead if this is possible.

clang/test/CXX/dcl.decl/dcl.meaning/dcl.fct.default/p7.cpp
40

I wish I could. Unfortunately [[@LINE-5]] is a CHECK line construct. Is it possible to refer to a relative line in a regex verify line?

riccibruno marked 2 inline comments as done.Sep 2 2020, 10:18 AM

Ah, I guess Ianded on an older version of this through a link and didn't notice. Nevermind.

riccibruno retitled this revision from [clang] Look through bindings when checking whether a default argument references a local entity. to [clang] Also consider structured bindings when checking whether a local entity is odr-used in a default argument..Sep 4 2020, 4:08 AM
riccibruno edited the summary of this revision. (Show Details)