<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 19, 2014 at 11:03 AM, 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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Nov 18, 2014 at 10:50 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">+cfe-commits<br><br>Though I've had trouble applying the summary patch you posted - could you rebase/repost? Sorry for the delays :/</div></blockquote><div><br></div></span><div>No worries. </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 18, 2014 at 10:49 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Nov 5, 2014 at 1:10 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: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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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 dir="ltr">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>/* unindented the longer part of the code */<br></div></blockquote><div><br></div></span><div>Fixed. Thanks for catching that! </div><span><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"><br>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></blockquote><div><br></div></span><div>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.</div><span><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"><br>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></blockquote><div><br></div></span><div>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></div></div></div></div></blockquote><div><br></div></span><div>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).</div><span><div> </div><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><span><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"><br>EmptyLookupTypoDiags::operator() - some {} on single-statement blocks (if (Ctx)/else)<br>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></blockquote><div><br></div></span><div>Done and done. </div><span><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"><br>std::string CorrectedStr(TC.getAsString(SemaRef.getLangOpts()));<br><br>Prefer copy/move init rather than direct init:<br><br>std::string CorrectedStr = TC.getAsString(SemaRef.getLangOpts());<br></div></blockquote><div><br></div></span><div>Changed. </div><span><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"><br>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></blockquote><div><br></div></span><div>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).</div></div></div></div></blockquote></span><div><br>Cool cool.<br> </div><span><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"><span><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"><br>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></blockquote><div><br></div></span><div>You're right, neither seem to be needed--though I thought I had added the return type because of clang complaining without it.</div><span><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"><br>Rather than goto in ParseCXXIdExpression, could that be a loop?<br></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote></span><div><br>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.<br><br>As for other ways to write this... one alternative, which I wouldn't necessarily espouse as better than your suggested loop:<br><br> bool first = true;<br> do {<br> if (!first)<br> UnconsumeToken()<br> first = false;<br> ...<br> } while (!Result.isInvalid() && !Result.get())<br><br>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:<br><br> while (true) {<br> ...<br> if (...)<br> break;<br> UnconsumeToken();<br> }<br><br>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.<br></div></div></div></div></blockquote></div></div></div></div></blockquote><div><br></div></div></div><div>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).</div></div></div></div></blockquote><div><br>An assertion sounds good, in any case.<br><br>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?<br><br>func()<br>if (failed)<br> func(other stuff)<br>assert(!failed)<br><br>?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><span><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"><span><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"><br><br>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></blockquote><div><br></div></span><div>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...</div></div></div></div></blockquote></span><div><br>Hmm, that's interesting - I'd be curious to understand why that is, but I guess it's got a good reason.<br></div></div></div></div></blockquote></div></div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br>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'<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><div><div><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><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"><br><br><br><br><br></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>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></div></div><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><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></div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<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></blockquote></div><br></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>