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

[David Blaikie]

+cfe-commits

Though I've had trouble applying the summary patch you posted - could you
rebase/repost? Sorry for the delays :/

On Tue, Nov 18, 2014 at 10:49 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Wed, Nov 5, 2014 at 1:10 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>>
>>
>> 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".)
>>
>
> Sure - up to you. I usually don't even think about what's being captured
> and just write [&] by default. (this applies really well to functions that
> will only call the lambda within the function (not storing it away to call
> later - at which point you get into interesting lifetime situations, etc) -
> for those, it's trickier and depends on the lifetime of the things being
> captured as to whether to capture by ref or value, etc).
>
>
>>
>>> 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).
>>
>
> Cool cool.
>
>
>>
>>> 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 - fair point. I'd probably at least still write it using the
> while(true) loop rather than goto, just to demonstrate to the reader what
> kind of control flow is happening here more easily.
>
> As for other ways to write this... one alternative, which I wouldn't
> necessarily espouse as better than your suggested loop:
>
>   bool first = true;
>   do {
>     if (!first)
>       UnconsumeToken()
>     first = false;
>     ...
>   } while (!Result.isInvalid() && !Result.get())
>
> Though calling '!Result.isInvalid() && !Result.get()' twice probably isn't
> the end of the world either, though certainly feels a bit off. Your
> suggestion of:
>
>   while (true) {
>     ...
>     if (...)
>       break;
>     UnconsumeToken();
>   }
>
> seems good. It's probably about as obvious as its going to get, I'd
> imagine. If the body of the loop is long, it might be more clear to pull it
> into a function and that'd make the loop's scope and purpose clearer.
>
>
>>>
>>> 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...
>>
>
> Hmm, that's interesting - I'd be curious to understand why that is, but I
> guess it's got a good reason.
>
>
>>
>>>
>>>
>>>
>>>
>>>
>>> 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/20141118/830d782e/attachment.html>