Cool. I'll go ahead and submit the patchset with that patch merged in so that it uses a helper method instead of a goto.
On Thu, Nov 20, 2014 at 1:47 PM, David Blaikie <[email protected]> wrote: > > > On Thu, Nov 20, 2014 at 1:31 PM, Kaelyn Takata <[email protected]> wrote: > >> >> >> On Wed, Nov 19, 2014 at 3:36 PM, David Blaikie <[email protected]> >> wrote: >> >>> >>> >>> On Wed, Nov 19, 2014 at 11:03 AM, Kaelyn Takata <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Tue, Nov 18, 2014 at 10:50 AM, David Blaikie <[email protected]> >>>> 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 <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Wed, Nov 5, 2014 at 1:10 PM, Kaelyn Takata <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, Nov 4, 2014 at 4:54 PM, David Blaikie <[email protected]> >>>>>>> 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). >>>> >>> >>> An assertion sounds good, in any case. >>> >>> If it's only a single retry - perhaps breaking out the body of the loop >>> into a function that you call twice would make sense? >>> >>> func() >>> if (failed) >>> func(other stuff) >>> assert(!failed) >>> >>> ? >>> >> >> I've attached a patch that replaces the goto in favor of a helper method >> for the part of ParseCXXIdExpression that has to be retried when correcting >> a keyword typo (it had to be a method instead of a static function because >> almost all of the Parser members being accessed are private). I'm fine with >> committing either version. >> > > Yep, that patch looks fine - thanks! > > >> >>> >>>> >>>>>> >>>>>>>> >>>>>>>> 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. >>>> >>> >>> Hmm, OK - that makes sense. Could/should we then do the correction on >>> the LHS/RHS only when a failure is about to cause the expression to be >>> thrown out? (that way if there's no error... oh, but of course there' >>> >> >> Yup, typo correction is being attempted in the places like >> ParseRHSOfBinaryExpression only when there is already an error and the >> original expression (including any TypoExprs within it) are about to be >> lost. ;) >> >>> >>> >>>> >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Wed, Oct 29, 2014 at 12:49 PM, Kaelyn Takata <[email protected]> >>>>>>>> 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 >>>>>>>>> [email protected] >>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
