r194002 - Try to correct a mistyped "-" or ">" to "->" for some C++ cases.

[Richard Smith]

On Thu, Nov 7, 2013 at 2:15 PM, Kaelyn Uhrain <rikka at google.com> wrote:

> Richard,
>
> Thanks for the review feedback!
>
> 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).
>

This looks a lot cleaner, thanks. I have a handful of somewhat minor
concerns:


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.

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.

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.


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.


Tiny things:
 * You should use getAs<RecordType>, not getAsStructureType() -- this
should work for classes and unions, too.
 * Use TentativeParsingAction rather than using PP.*Backtrack* directly --
this will fix a minor bug where you get the source location of the '->'
token wrong.
 * Can you add an RAII object for registering and unregistering the
DelayingDiagnosticConsumer?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131111/25737b35/attachment.html>