Page MenuHomePhabricator

Warn when ScopeDepthOrObjCQuals overflows
AcceptedPublic

Authored by Mordante on Jun 29 2019, 11:08 AM.

Details

Reviewers
rsmith
rjmccall
Summary

Before when the overflow occurred an assertion as triggered. Now check
whether the maximum has been reached and warn properly.

This patch fixes the original submission of bug 19607. The part mentioned
in its 'duplicate' bug 33162 has not been fixed. I want to look at that
after this patch has been accepted.

Diff Detail

Event Timeline

Mordante created this revision.Jun 29 2019, 11:08 AM
rjmccall added inline comments.
clang/lib/Parse/ParseDecl.cpp
6600

Comment indentation.

Should we do this when starting to parse a function prototype instead of when parsing a parameter?

Mordante marked an inline comment as done.Jul 11 2019, 2:45 PM
Mordante added inline comments.
clang/lib/Parse/ParseDecl.cpp
6600

Thanks I noticed I forgot to change the tabs to spaces.

I looked at your suggestion to move the code, but I think this is the proper place. Now it also validates whether lambdas exceed the limit, else we need to check at two places.
I'll also add a unit test to test for the lambda.

Mordante updated this revision to Diff 209348.Jul 11 2019, 2:47 PM

tab -> space
adds an extra unit test for lambdas
fixes an off by one error found while testing the lambdas

rjmccall added inline comments.Jul 11 2019, 8:01 PM
clang/lib/Parse/ParseDecl.cpp
6600

I don't understand. I'm just saying to put the check at the top of the function instead of inside the loop.

Mordante marked an inline comment as done.Jul 12 2019, 9:20 AM
Mordante marked an inline comment as done.
Mordante added inline comments.
clang/lib/Parse/ParseDecl.cpp
6600

I now understand what you mean. I'll upload a new patch.

Mordante updated this revision to Diff 209513.Jul 12 2019, 9:25 AM

Moved the test out of the loop as suggested by rjmccall.

Thanks.

It's good to have a lambda test, but that one isn't actually testing the lambda path — the place the diagnostic will trigger is just the normal function-prototype path, just originally within a lambda. You can do something like this:

template <class T> int foo(T &&t);
void bar(int x = foo(
    [](int x = foo(
    [](int x = foo(
    [](int x = foo(
    ...

It looks like you'll have to bump -fbracket-level to get a crash, but since that's configurable and this limit isn't, we should still be testing that.

Also I think the same problem can happen with the "blocks" extension — could you test that, too? That would be something like (with -fblocks):

void bar(int x = foo(
    ^(int x = foo(
    ^(int x = foo(
    ^(int x = foo(
    ...

(Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.)

Mordante updated this revision to Diff 209703.Jul 13 2019, 10:44 AM

Addresses @rjmccall's remarks.
Fixes the tests for the nested lambda's.
As suspected the blocks also have the same nesting limit, thus added a test for them.

rjmccall accepted this revision.Jul 13 2019, 11:32 AM

Thanks, LGTM!

This revision is now accepted and ready to land.Jul 13 2019, 11:32 AM

Thanks for the review. I don't have SVN access, can you commit these changes?