<div dir="ltr">BTW, here is just this patch from the set, with all of the changes discussed and rebased against ToT.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 5, 2014 at 1:14 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Accidentally hit "Reply" instead of "Reply All".<div><div class="h5"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 4, 2014 at 4:54 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span>Could you post/send (on/off list, etc) an aggregate patch so I can play around with the behavior of this patch?<br><br>Some superficial patch review<br><br>lambda passed to CorrectDelayedTyposInExpr has 'else-after-return' - you could move the short block up to the top:<br><br>if (!...CPlusPlus11)<br> return this->VerifyIntegerConstantExpression(E);<br><div dir="ltr">/* unindented the longer part of the code */<br></div></span></div></div></div></blockquote><br>Fixed. Thanks for catching that! <br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><div dir="ltr">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.<br></div></span></div></div></div></blockquote><br>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.<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><div dir="ltr">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)<br></div></span></div></div></div></blockquote><br>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".)<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br>EmptyLookupTypoDiags::operator() - some {} on single-statement blocks (if (Ctx)/else)<br><div dir="ltr">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.<br></div></span></div></div></div></blockquote><br>Done and done. <br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br>std::string CorrectedStr(TC.getAsString(SemaRef.getLangOpts()));<br><br>Prefer copy/move init rather than direct init:<br><br><div dir="ltr">std::string CorrectedStr = TC.getAsString(SemaRef.getLangOpts());<br></div></span></div></div></div></blockquote><br>Changed. <br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><div dir="ltr">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?<br></div></span></div></div></div></blockquote><br>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).<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><div dir="ltr">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.<br></div></span></div></div></div></blockquote><br>You're right, neither seem to be needed--though I thought I had added the return type because of clang complaining without it.<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><div dir="ltr">Rather than goto in ParseCXXIdExpression, could that be a loop?<br></div></span></div></div></div></blockquote><br>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.<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div class="gmail_extra"><div class="gmail_quote"><span><br><br><div dir="ltr">Hmm - why is the correction applied to the LHS/RHS of the binary expression, rather than waiting until the full expression is parsed?<br></div></span></div></div></div></blockquote><br>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...<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><br><br><br><br><br><br>On Wed, Oct 29, 2014 at 12:49 PM, Kaelyn Takata <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><br>Among other things, this eliminates at least one redundant error case<br>and allows recovery in several cases where it wasn't possible before<br>(e.g. correcting a mistyped static_cast<>).<br>---<br> include/clang/Parse/Parser.h | 9 +++<br> include/clang/Sema/Sema.h | 4 +-<br> lib/Parse/ParseCXXInlineMethods.cpp | 1 +<br> lib/Parse/ParseDecl.cpp | 4 +-<br> lib/Parse/ParseDeclCXX.cpp | 2 +-<br> lib/Parse/ParseExpr.cpp | 30 +++++++---<br> lib/Parse/ParseExprCXX.cpp | 13 ++++-<br> lib/Parse/ParseObjc.cpp | 5 +-<br> lib/Parse/ParseOpenMP.cpp | 3 +-<br> lib/Parse/ParseStmt.cpp | 8 ++-<br> lib/Sema/SemaExpr.cpp | 110 ++++++++++++++++++++++++++++++++---<br> lib/Sema/SemaStmt.cpp | 19 ++++++<br> test/FixIt/fixit-unrecoverable.cpp | 4 +-<br> test/SemaCXX/typo-correction.cpp | 7 ++-<br> test/SemaTemplate/crash-10438657.cpp | 2 +-<br> 15 files changed, 189 insertions(+), 32 deletions(-)<br><br><br><blockquote class="gmail_quote" style="margin:0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;border-right-width:1px;border-right-color:rgb(204,204,204);border-right-style:solid;padding-left:1ex;padding-right:1ex"></blockquote>_______________________________________________<br><blockquote class="gmail_quote" style="margin:0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;border-right-width:1px;border-right-color:rgb(204,204,204);border-right-style:solid;padding-left:1ex;padding-right:1ex"></blockquote>cfe-commits mailing list<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
</blockquote><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
</blockquote><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>