[PATCH v2 6/6] Wire up delayed typo correction to DiagnoseEmptyLookup and set up Sema::ActOnIdExpression to use the new functionality.

[Kaelyn Takata]

Accidentally hit "Reply" instead of "Reply All".

On Tue, Nov 4, 2014 at 4:54 PM, David Blaikie <dblaikie at gmail.com> wrote:

> Could you post/send (on/off list, etc) an aggregate patch so I can play
> around with the behavior of this patch?
>
> Some superficial patch review
>
> lambda passed to CorrectDelayedTyposInExpr has 'else-after-return' - you
> could move the short block up to the top:
>
> if (!...CPlusPlus11)
>   return this->VerifyIntegerConstantExpression(E);
> /* unindented the longer part of the code */
>

Fixed. Thanks for catching that!

>
> I'm slightly surprised at the indentation there too - I think clang-format
> has a special case for lambdas as the last function argument, where it
> won't indent them extra (so the "});" will be at the same level as the
> expression - maybe it only does that if you're not initializing a variable?
> Dunno.
>

I think it was a quirk in the version of clang-format used to originally
format this several months ago; clang-format now formats it a bit
differently.

>
> Do you need the 'this->' qualification in that lambda? I assume not. (& I
> usually favor just capture everything by reference [&] - but up to you on
> that, we're all still certainly feeling out how best to write C++11)
>

I didn't think the implicit "this" could be used for accessing member
functions... I just learned something new. :) (Though "this" still needs to
be in the capture list--I didn't capture everything by reference because
the only variable that needed to be captured was "this".)

>
> EmptyLookupTypoDiags::operator() - some {} on single-statement blocks (if
> (Ctx)/else)
> Might be worth inverting the if/following block - the if block is
> longer/nested than the tail block, so switching them around should reduce
> indentation, etc.
>

Done and done.

>
> std::string CorrectedStr(TC.getAsString(SemaRef.getLangOpts()));
>
> Prefer copy/move init rather than direct init:
>
> std::string CorrectedStr = TC.getAsString(SemaRef.getLangOpts());
>

Changed.

>
> Hmm - it might be nice to write EmptyLookupTypoDiags just as a single free
> function (taking all those members as parameters instead) then call that in
> a lambda - that way you don't have to write all the member state/ctor/etc
> logic?
>

I had to capture variables by value instead of by reference to avoid
lifetime issues (and having 74 tests fail) and create a local variable so
the LookupResult didn't have to be captured since it cannot be copied, but
I did get it working as a static function and a lambda and the switch
resulted in a net change of 17 fewer lines (37 added, 54 deleted).

>
> lambda in ParseCaseStatement: I'd probably write "return ExprResult(...)"
> rather than explicitly specifying the return type. (do you even need to do
> either? If CorrectDelayedTyposInExpr is a template, then its return result
> should just implicitly convert to ExprResult in the caller, if it takes a
> std::function, that should still work too... - I can explain this more in
> person, perhaps.
>

You're right, neither seem to be needed--though I thought I had added the
return type because of clang complaining without it.

>
> Rather than goto in ParseCXXIdExpression, could that be a loop?
>

The cleanest way I can see to write it as a loop would be to wrap it in a
"while (true) {...}" loop, and have an "if (Result.isInvalid() ||
Result.get()) break;" right before the (now unconditional) call to
UnconsumeToken. The only other way I can see to do the loop would require
checking "!Result.isInvalid() && !Result.get()" twice in a row, once to
know whether to call UnconsumeToken, and once in the do...while loop to
know whether to continue looping.

>
>
> Hmm - why is the correction applied to the LHS/RHS of the binary
> expression, rather than waiting until the full expression is parsed?
>

If you are referring to within ParseRHSOfBinaryExpression, it's because any
error in the RHS will cause the LHS to be replaced with an ExprError
without any typos in the LHS being diagnosed, so waiting until the full
expression is parsed causes typos to go undiagnosed if there are unrelated
parse errors in the full expression. This had been a bit of a pain to
figure out and track down why typo correction on certain full expressions
wasn't happening for part of the expression...

>
>
>
>
>
>
> On Wed, Oct 29, 2014 at 12:49 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>
>> Among other things, this eliminates at least one redundant error case
>> and allows recovery in several cases where it wasn't possible before
>> (e.g. correcting a mistyped static_cast<>).
>> ---
>>  include/clang/Parse/Parser.h         |   9 +++
>>  include/clang/Sema/Sema.h            |   4 +-
>>  lib/Parse/ParseCXXInlineMethods.cpp  |   1 +
>>  lib/Parse/ParseDecl.cpp              |   4 +-
>>  lib/Parse/ParseDeclCXX.cpp           |   2 +-
>>  lib/Parse/ParseExpr.cpp              |  30 +++++++---
>>  lib/Parse/ParseExprCXX.cpp           |  13 ++++-
>>  lib/Parse/ParseObjc.cpp              |   5 +-
>>  lib/Parse/ParseOpenMP.cpp            |   3 +-
>>  lib/Parse/ParseStmt.cpp              |   8 ++-
>>  lib/Sema/SemaExpr.cpp                | 110
>> ++++++++++++++++++++++++++++++++---
>>  lib/Sema/SemaStmt.cpp                |  19 ++++++
>>  test/FixIt/fixit-unrecoverable.cpp   |   4 +-
>>  test/SemaCXX/typo-correction.cpp     |   7 ++-
>>  test/SemaTemplate/crash-10438657.cpp |   2 +-
>>  15 files changed, 189 insertions(+), 32 deletions(-)
>>
>>
>> _______________________________________________
>>
>> cfe-commits mailing list
>>
>>> cfe-commits at cs.uiuc.edu
>>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141105/d923fd7a/attachment.html>