Page MenuHomePhabricator

[clang] SequenceChecker: Also visit default arguments and default initializers.
Needs ReviewPublic

Authored by riccibruno on Jun 2 2020, 6:51 AM.

Details

Reviewers
rsmith
Summary

SequenceChecker is currently not visiting default arguments and default initializers.
It is therefore missing cases like:

int a;
int foo(int x = a++, int y = a);
void test() {
  foo(); // should warn
  foo(a++); // idem
}

or

int a;
struct X { int b = ++a; }; // aggregate in C++14
void test() {
  int c = X{}.b + a; // should warn in C++14
}

This patch modify SequenceChecker to visit default arguments and default initializers.

We additionally maintain a chain of additional source location information about where
default arguments and/or default initializers were used. Most of the time only one element
will be present in the chain but in general we might have to show a path through default
arguments and/or default initializers.

Diff Detail

Event Timeline

riccibruno created this revision.Jun 2 2020, 6:51 AM
riccibruno retitled this revision from [clang][Sema] SequenceChecker: Also visit default arguments. to [clang] SequenceChecker: Also visit default arguments..Jun 11 2020, 3:19 AM
rsmith accepted this revision.Tue, Jun 16, 5:29 PM

Seems reasonable. I think we need similar handling for CXXDefaultInitExpr, for cases like this:

int a;
struct X { int b = ++a; };
int c = X{} + a;
clang/test/SemaCXX/warn-unsequenced.cpp
282–283

I think it's important that we handle this in the near future (though I don't mind if you'd like to submit this patch as-is and deal with this as a follow-on). For a case like:

void f(int, int = a++);
// ... some time later ...
f(a);

... a warning that only gives the location of the default argument is not useful. We need to show both locations (and potentially a path through multiple default arguments, I suppose; yuck).

This revision is now accepted and ready to land.Tue, Jun 16, 5:29 PM
riccibruno updated this revision to Diff 271464.EditedWed, Jun 17, 1:19 PM
riccibruno retitled this revision from [clang] SequenceChecker: Also visit default arguments. to [clang] SequenceChecker: Also visit default arguments and default initializers..
riccibruno edited the summary of this revision. (Show Details)

Ah, I forgot about default initializers. Thanks!

I have updated the patch to track chains of default arguments and/or initializers.
This shouldn't be too expensive since this will only be used when traversing a
CXXDefaultArgExpr or CXXDefaultInitExpr. Still I am running my usual benchmark
(all of boost) and will report with the results.

I think that this uncovers another issue (test_default_init in the test), but I am not sure whether the C++17 or C++11 behavior is correct.

riccibruno marked 3 inline comments as done.Wed, Jun 17, 1:24 PM
riccibruno added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2124

I should just use %select here.

riccibruno marked an inline comment as done.Wed, Jun 17, 1:33 PM
riccibruno added inline comments.
clang/test/SemaCXX/warn-unsequenced.cpp
313

I meant "constructor initializers" instead, but I am not sure if they should be visited.

riccibruno marked an inline comment as done.Wed, Jun 17, 2:02 PM
riccibruno added inline comments.
clang/test/SemaCXX/warn-unsequenced.cpp
313

Ah, struct X { int b = ++a; }; is not an aggregate in C++11 so I think this is fine.
I will remove the comment. Sorry for the spam.

Remove the FIXME comment in the default_init test. The behavior is actually fine since struct S { int b = ++a; }; is not an aggregate in C++11 but is an aggregate in C++17.

  • Remove a left-over FIXME in the tests.
  • Use a %select in the diagnostic.
  • Pack AdditionalSourceLocationData into two unsigneds.
  • Simplify the chain walking code in checkUsage.
  • Mark the patch as needing review since the code has changed substantially since the original patch.
riccibruno requested review of this revision.Thu, Jun 18, 4:31 AM
riccibruno edited the summary of this revision. (Show Details)Thu, Jun 18, 1:28 PM

@rsmith

I think it's important that we handle this in the near future (though I don't mind if you'd like to submit this patch as-is and deal with this as a follow-on). For a case like:

void f(int, int = a++);
 // ... some time later ...
 f(a);

... a warning that only gives the location of the default argument is not useful. We need to show both locations (and potentially a path through multiple default arguments, I suppose; yuck).

I have modified this patch to maintain a chain of additional source location information as we visit default arguments and/or initializers. Each element in this chain stores the location of where the default argument and/or initializer was used. This chain is then displayed after a warning is emitted.

Does this address your concern?

riccibruno edited the summary of this revision. (Show Details)Sat, Jul 4, 3:41 PM

@rsmith I have modified this patch to address your comment above (we should track where default argument(s) and/or default member initializer(s) were used). You accepted this originally but I would like to make sure that you are happy with the updated patch.

I was also confused for a while about the example:

int a;
struct S { int b = ++a; };
void Test() {
  int c = S{}.b + a; // Warn in C++14 and above, but not before.
}

I *think* that this should warn in C++14 and above, because S is an aggregate in C++14 and above, and the default member initializers of an aggregate are also part of the full-expression containing the initialization of the aggregate ([intro.execution]p12).