<div dir="ltr">On Mon, Nov 11, 2013 at 5:10 PM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br><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 class="im">On Mon, Nov 11, 2013 at 4:13 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</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"><div><div>On Thu, Nov 7, 2013 at 2:15 PM, Kaelyn Uhrain <span dir="ltr"><<a href="mailto:rikka@google.com" target="_blank">rikka@google.com</a>></span> wrote:<br>

</div></div><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"><div>Richard,</div><div><br></div><div>Thanks for the review feedback!</div>


<div><br></div>Here's an alternate version that better handles cases (such as the example you gave) where the code is actually valid without the "-" or ">" being changed to "->", by only trying the correction if an error diagnostic would have been emitted. It is similar to what I had mentioned doing, except that if the correction fails, ParseRHSOfBinaryExpr is called a second time to emit the original diagnostic messages. I ended up doing it this way because I encountered a problem with my previous idea: how to get the token stream in the right state if the correction fails and the original errors should be emitted. Preprocessor::EnableBacktrackAtThisPos() and friends work for resetting the token stream after the call to ParseRHSOfBinaryExpr but don't quite work for setting the token stream back to the state after the (first) call to ParseRHSOfBinaryExpr after trying the correction and having it fail (emitting the stored diagnostics from the call to ParseRHSOfBinaryExpr to avoid having to call it a second time only works if the token stream can also be restored to the correct state).</div>


</blockquote><div><br></div></div></div><div>This looks a lot cleaner, thanks. I have a handful of somewhat minor concerns:</div><div><br></div><div><br></div><div>DelayingDiagnosticConsumer doesn't appropriately handle DiagnosticConsumer::IncludeInDiagnosticCounts. "clang-check -fixit" is supposed to return 0 if it fixes all errors; it looks like it won't if it fixes this error, for this reason. This is a bit tricky: you don't know whether to include the diagnostic in diagnostic counts until you know whether you're emitting it, and you don't know that until you know whether you can recover.  </div>

</div></div></div></blockquote><div><br></div></div><div>In which circumstances is the diagnostic count not being handled correctly? DelayingDiagnosticConsumer simply passes all the diagnostics it captured on to a regular DiagnosticConsumer in the event those diagnostics should be emitted; unless I'm missing something it is just acting as a buffer/queue in front of another DiagnosticConsumer, and the latter should be handling the diagnostic counts. (But this and all of the other points you brought up are why I wanted and appreciate the code review! All this mucking around with the parser and diagnostics is a bit outside familiar territory.)</div>
</div></div></div></blockquote><div><br></div><div>The diagnostic counts are handled by the caller of DiagnosticConsumer, not by the DiagnosticConsumer itself, and the caller chooses what to do based on the result of that virtual function. I don't think you can decide what to return from there until you know whether you're going to emit the diagnostic (which in turn depends on whether an error occurs later).</div>
<div><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 class="im"><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>You're suppressing diagnostics that come from outside the immediate context of the error (for instance, if the RHS triggers a template instantiation, you shouldn't suppress diagnostics from there, because they won't be emitted if the code later uses the same template specialization again). That means that you can get a --fixit run that fixes all errors, and yet rebuilding the code still produces errors; that's not really supposed to happen.</div>

</div></div></div></blockquote><div><br></div></div><div>Yeah, the case of there being errors in the initial code path and the correction causing errors and sticking with the original set of errors leaves a lot to be desired. I'd prefer to be able to use a second DelayingDiagnosticConsumer to catch the errors from trying the correction and on failure just emit the original diagnostics and set the parser to the correct state, but couldn't figure out a way to achieve that with the current tentative parsing/backtracking facilities.</div>
</div></div></div></blockquote><div><br></div><div>Right, that would definitely help.</div><div> </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>It would still leave the issue of cleaning up template instantiations triggered by whichever path wasn't taken that weren't triggered by the path that was taken. And in those cases, the (suppressed) diagnostics from the instantiations would be bogus anyway as the resulting state is that the instantiations shouldn't have ever occurred.</div>
</div></div></div></blockquote><div><br></div><div>The case of suppressing diagnostics from the '-' or '>' when doing the '->' fixup is the problematic one: in that case, we could use the RHS of the '-' or '>' again later in the same source file, and we'd fail to diagnose the incorrect code.</div>
<div><br></div><div>You could check whether a template is being instantiated from your diagnostic handler, and not suppress the diagnostic in that case, when handling the '-' or '>' case. If you hit such a diagnostic, you shouldn't do the '->' recovery, since you've already diagnosed the other form, and the '->' recovery probably won't be appropriate anyway. (You should move the diagnostic consumer over to include/clang/Sema/... if you do this, though, since the Parser shouldn't be directly poking at such things.)</div>
<div><br></div><div>In the case where the '->' parse suppresses instantiation diagnostics, we'd only have a problem if we couldn't recover from the '-' or '>' parse failure, so you should probably only enter that codepath if the '-' or '>' parse fails *and* at least one error has no fix-it.</div>
<div><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 class="im"><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>I'm also a little concerned about the cost of turning on tentative parsing/backtracking here. Enabling tentative parsing (allegedly!) has a nontrivial cost, but I suppose we won't see pointers to class types on the LHS of '-' or '>' very frequently.</div>

</div></div></div></blockquote><div><br></div></div><div>Yeah, both this version and my original patch were based on the assumption that pointers to classes wouldn't be common on the LHS of '-' or '>' and so incurring a bit of overhead to be able to catch and diagnose a mistyped "->" wouldn't be a huge compile-time issue.</div>
<div class="im">
<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><br></div><div><br></div><div>I don't really have good suggestions for the above -- this is a really tricky issue to recover from, and we're not really set up for tentatively performing arbitrary parsing actions.</div>


<div><br></div><div><br></div><div>Tiny things:</div><div> * You should use getAs<RecordType>, not getAsStructureType() -- this should work for classes and unions, too.</div></div></div></div></blockquote><div><br>

</div></div><div>Not sure about unions, but getAsStructureType() seems to work just fine for classes. I will change it though... I always err toward using getAsStructureType instead of getAs<RecordType> whenever I look at the API docs as they don't indicate the former is deprecated and I always feel like it may have some extra logic that is needed but not provided by getAs<>. (I also tend forget that getAs is a template function as the doxygen docs render such things poorly; e.g. in  <a href="http://clang.llvm.org/doxygen/classclang_1_1Type.html" target="_blank">http://clang.llvm.org/doxygen/classclang_1_1Type.html</a> the "template<typename T >" is grey instead of black and in the far left of the column with return types separated by a large amount of whitespace from the "const T* getAs()" and actually on the line above the return type... and the 1-line blurb for getAs in the "Public Member Functions" table is rather misleading: "This will check for a TypedefType by removing any existing sugar until it reaches a TypedefType or a non-sugared type."... but I digress.)</div>
</div></div></div></blockquote><div><br></div><div>Weird. The implementation looks like it would only work for 'struct', not for 'class'. And yeah, wow, that documentation is *bad*. Looks like doxygen is putting the documentation for the specializations of getAs before the documentation for the primary template.</div>
<div> </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 class="im"><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> * Use TentativeParsingAction rather than using PP.*Backtrack* directly -- this will fix a minor bug where you get the source location of the '->' token wrong.</div></div></div></div></blockquote><div><br>

</div></div><div>Ah ok, I'll look into that. I found the PP.*Backtrack* functions long before TentativeParsingAction when trying to figure out how to get the parser or lexer to backtrack.</div><div class="im"><div><br>
</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> * Can you add an RAII object for registering and unregistering the DelayingDiagnosticConsumer?</div></div></div></div>
</blockquote></div></div><br></div><div class="gmail_extra">Sure thing.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks again for reviewing these patches!</div><div class="gmail_extra"><br></div><div class="gmail_extra">

Cheers,</div><div class="gmail_extra">Kaelyn</div></div>
</blockquote></div><br></div></div>