[PATCH v2 6/6] Wire up delayed typo correction to DiagnoseEmptyLookup and set up Sema::ActOnIdExpression to use the new functionality.
Kaelyn Takata
rikka at google.com
Mon Nov 17 15:57:01 PST 2014
BTW, here is just this patch from the set, with all of the changes
discussed and rebased against ToT.
On Wed, Nov 5, 2014 at 1:14 PM, Kaelyn Takata <rikka at google.com> wrote:
> 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/20141117/5566e496/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: v3-0006-Wire-up-delayed-typo-correction-to-DiagnoseEmptyL.patch
Type: text/x-patch
Size: 20992 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141117/5566e496/attachment.bin>
More information about the cfe-commits
mailing list