+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 <[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. > > >>> >>> 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 <[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
