[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
Wed Nov 19 11:03:32 PST 2014
On Tue, Nov 18, 2014 at 10:50 AM, David Blaikie <dblaikie at gmail.com> wrote:
> +cfe-commits
>
> Though I've had trouble applying the summary patch you posted - could you
> rebase/repost? Sorry for the delays :/
>
No worries.
>
> 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.
>>
>
I just remembered the other reason why I wrote this as a goto instead of a
loop (and why the goto label is "retry"): the body of the "loop" should in
most cases only be executed once, would only be executed a second time if
there is an error and typo correction suggests a keyword, and should
*never* be executed more than twice (i.e. the goto should only ever be
called at most once) unless something has gone horribly, impossibly wrong
("wrong" being a typo was encountered, a keyword was suggested,
substituting the keyword causes typo correction to be triggered again in
the same spot and suggest a keyword again... a situation that shouldn't be
possible and for which I'd be willing to add an assertion to prevent it
from happening).
>
>>
>>>>
>>>> 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.
>>
>
I think it's because errors in the expression are communicated through the
same channel as the expression itself: the ExprResult. So for something
that combines multiple expressions into one (which it seems that
ParseRHSOfBinaryExpression handles both parsing the RHS and combining the
LHS and RHS), it can either return the subexpression tree or an
ExprError.... which is fine up until something more needs to be done with
the subexpression tree once an ExprError has been generated.
>
>>
>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> 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/20141119/59f1fc78/attachment.html>
More information about the cfe-commits
mailing list