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
