Differential D36357
Added a better diagnostic when using the delete operator with lambdas Authored by Rakete1111 on Aug 5 2017, 8:10 AM.
Details
This adds a new error for missing parentheses around lambdas in delete operators. int main() {
delete []() { return new int(); }();
}This will result in: test.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
delete []() { return new int(); }();
^~~~~~~~~
test.cpp:2:9: note: add parentheses around the lambda
delete []() { return new int(); }();
^
( )
Diff Detail
Event Timeline
Comment Actions Did you forget to upload the updated patch? This looks unchanged compared to the prior version. Comment Actions
Whoops, that's true. Sorry... Comment Actions @rsmith Which type specifiers should I test for? just T? Or also T*, T&, ...? Now I'm just checking for an identifier, but anything else would make the lookahead more complicated I think. Any ideas? Comment Actions @rsmith Should I only check for an identifier, like only "int" and nothing else? Because parsing a full type specifier might be a bit expensive, no?
Comment Actions Thanks!
Comment Actions This looks good, with some minor changes. Please add more test coverage, though, specifically:
Comment Actions Addressed review comments. Note that clang doesn't support the fourth kind of lambda yet ([]<>), because D36527 hasn't been merged yet, so I didn't add a test case for that one. Comment Actions Thanks! Some minor nits, please feel free to commit once they're addressed. We support that now, so please add a test for that :)
Comment Actions @rsmith One last question: The fixit diagnostic seems to be inconsistent with the rest? main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'
delete[] { return new int; }
^~~~~~~~
( )Shouldn't the ^~~~~~~ be starting at the []? Comment Actions Is this also okay? main.cpp:2:9: warning: lambda expressions are incompatible with C++98 [-Wc++98-compat]
delete[] { return new int; }
^
main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]'; add parentheses to treat this as a lambda-expression
delete[] { return new int; }
^~~~~~~~
( )Comment Actions Pointing the caret at the [ might be clearer, but I think that's fine too. Hmm. It would be better to produce the error first, but that would make it more difficult to get the fixit hint right. I suppose in the case where we've already decided we're going to diagnose, we could perform a tentative parse and skip to the } of the lambda. Comment Actions
How should I do this? Do I just skip to the next }, or also take into account any additional scopes? Also does this mean that I skip and then revert, because that seems pretty expensive? Comment Actions It would be a little expensive, yes, but we'd only be doing it on codepaths where we're producing an error -- for an ill-formed program, it's OK to take more time in order to produce a better diagnostic. Skipping to the next } won't work, because SkipUntil will skip over pairs of brace-balanced tokens (so you'll skip past the } you're looking for), but skipping until the next { and then skipping to the } after it should work.
Comment Actions Hmm wouldn't this interact badly with {} in initializers? []<int = {0}> {};
[](int = {0}) {};
Comment Actions Ouch. The first of these two seems like it won't work, since SkipUntil has no idea the angles are brackets. =( Getting this right is ... really hairy. Eg: []<bool = a<b>{} // end of lambda here?
>(){} // or here?(The answer depends on whether a is a template.) And the same problem shows up in the trailing-return-type and the requires-clause. How about this: SkipUntil an { or <. If you find a < first, don't attach a FixItHint to the diagnostic since we can't easily tell where to suggest putting the ). | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please add to this: "; add parentheses to treat this as a lambda-expression" or similar.