[cfe-commits] r65570 - in /cfe/trunk: include/clang/Analysis/ include/clang/Basic/ include/clang/Driver/ include/clang/Parse/ lib/Analysis/ lib/Basic/ lib/Driver/ lib/Parse/ lib/Rewrite/ lib/Sema/ test/Parser/ test/SemaTemplate/

[Chris Lattner]

On Feb 26, 2009, at 1:00 PM, Douglas Gregor wrote:

> Author: dgregor
> Date: Thu Feb 26 15:00:50 2009
> New Revision: 65570
>
> URL: http://llvm.org/viewvc/llvm-project?rev=65570&view=rev
> Log:
> Introduce code modification hints into the diagnostics system. When we
> know how to recover from an error, we can attach a hint to the
> diagnostic that states how to modify the code, which can be one of:

Very nice Doug, as you know I'm a big fan of this work!

Please update the diag section of the internals doc to mention this.

> +++ cfe/trunk/include/clang/Basic/Diagnostic.h Thu Feb 26 15:00:50  
> 2009
> @@ -68,6 +68,69 @@
>     };
>   }
>
> +/// \brief Annotates a diagnostic with some code that should be
> +/// inserted, removed, or replaced to fix the problem.
> +///
> +/// This kind of hint should be used when we are certain that the
> +/// introduction, removal, or modification of a particular (small!)
> +/// amount of code will correct a compilation error. The compiler
> +/// should also provide full recovery from such errors, such that
> +/// suppressing the diagnostic output can still result successful
> +/// compilation.

"result *in* successful compilation"?

>
> +class CodeModificationHint {
> +public:
> +  /// \brief Tokens that should be removed to correct the error.
> +  SourceRange RemoveRange;
> +
> +  /// \brief The location at which we should insert code to correct
> +  /// the error.
> +  SourceLocation InsertionLoc;
> +
> +  /// \brief The actual code to insert at the insertion location,  
> as a
> +  /// string.
> +  std::string CodeToInsert;
> +
> +  /// \brief Empty code modification hint, indicating that no code
> +  /// modification is known.
> +  CodeModificationHint() : RemoveRange(), InsertionLoc() { }
> +
> +  /// \brief Create a code modification hint that inserts the given
> +  /// code string at a specific location.
> +  CodeModificationHint(SourceLocation InsertionLoc, const  
> std::string &Code)
> +    : RemoveRange(), InsertionLoc(InsertionLoc), CodeToInsert(Code)  
> { }
> +
> +  /// \brief Create a code modification hint that removes the given
> +  /// source range.
> +  CodeModificationHint(SourceRange RemoveRange)
> +    : RemoveRange(RemoveRange), InsertionLoc(), CodeToInsert() { }
> +
> +  /// \brief Create a code modification hint that replaces the given
> +  /// source range with the given code string.
> +  CodeModificationHint(SourceRange RemoveRange, const std::string  
> &Code)
> +    : RemoveRange(RemoveRange), InsertionLoc(RemoveRange.getBegin()),
> +      CodeToInsert(Code) { }

Instead of using ctors with different prototypes to mean magically  
different things, how about using static functions.  This gives:

CodeModificationHint::CreateReplacement(...)
CodeModificationHint::CreateRemoval(...)
CodeModificationHint::CreateInsertion(...)

instead of different magic protos.

>
> +};
> +
> +/// \brief Creates a code modification hint that inserts the given
> +/// string at a particular location in the source code.
> +inline CodeModificationHint
> +CodeInsertionHint(SourceLocation InsertionLoc, const std::string  
> &Code) {
> +  return CodeModificationHint(InsertionLoc, Code);
> +}
> +
> +/// \brief Creates a code modification hint that removes the given
> +/// source range.
> +inline CodeModificationHint CodeRemovalHint(SourceRange  
> RemoveRange) {
> +  return CodeModificationHint(RemoveRange);
> +}
> +
> +/// \brief Creates a code modification hint that replaces the given
> +/// source range with the given code string.
> +inline CodeModificationHint
> +CodeReplacementHint(SourceRange RemoveRange, const std::string  
> &Code) {
> +  return CodeModificationHint(RemoveRange, Code);

It also allows these to be removed?

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/include/clang/Basic/TokenKinds.h (original)
> +++ cfe/trunk/include/clang/Basic/TokenKinds.h Thu Feb 26 15:00:50  
> 2009
> @@ -44,6 +44,7 @@
> };
>
> const char *getTokenName(enum TokenKind Kind);
> +const char *getTokenSpelling(enum TokenKind Kind);

Please add a doxygen comment for these, describing their behavior.   
'getTokenSpelling' is a misleading name for this method, because  
tok::getTokenSpelling(Tok.getKind()) doesn't return the token's  
spelling.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/include/clang/Parse/Parser.h (original)
> +++ cfe/trunk/include/clang/Parse/Parser.h Thu Feb 26 15:00:50 2009
> @@ -37,6 +37,12 @@
>   /// that this is valid.
>   Token Tok;
>
> +  // PrevTokLocation - The location of the token we previously
> +  // consumed. This token is used for diagnostics where we expected  
> to
> +  // see a token following another token (e.g., the ';' at the end of
> +  // a statement).
> +  SourceLocation PrevTokLocation;

Have you measured the perf impact of adding this?  I expect that this  
is just as expensive as adding one-token lookahead to the parser, and  
is not something that should be done lightly.  Notably, this impacts  
*all* reads from the preprocessor, not just the ones that could need  
the info.

> +++ cfe/trunk/lib/Basic/TokenKinds.cpp Thu Feb 26 15:00:50 2009
> @@ -27,3 +27,66 @@
>   assert(Kind < tok::NUM_TOKENS);
>   return TokNames[Kind];
> }
> +
> +/// \brief Determines the spelling of simple punctuation tokens like
> +/// '!' or '%', and returns NULL for literal and annotation tokens.

Please repeat this in the header file, and mention how it handles the  
digraph issue etc.  Please explicitly say that if you want to preserve  
spelling in the digraph case that spelling should be used, not this  
method.

> +++ cfe/trunk/lib/Parse/ParseExpr.cpp Thu Feb 26 15:00:50 2009
> @@ -365,10 +365,19 @@
>
>     if (!LHS.isInvalid()) {
>       // Combine the LHS and RHS into the LHS (e.g. build AST).
> -      if (TernaryMiddle.isInvalid())
> +      if (TernaryMiddle.isInvalid()) {
> +        // If we're using '>>' as an operator within a template
> +        // argument list (in C++98), suggest the addition of
> +        // parentheses so that the code remains well-formed in C++0x.

It seems somewhat strange to me to handle this diag here instead of  
sema.  Are you sure it wouldn't be more elegant in sema?

> +++ cfe/trunk/lib/Parse/Parser.cpp Thu Feb 26 15:00:50 2009
> @@ -68,6 +68,28 @@
>   return Diag(Tok.getLocation(), DiagID);
> }
>
> +/// \brief Emits a diagnostic suggesting parentheses surrounding a
> +/// given range.
> +///
> +/// \param Loc The location where we'll emit the diagnostic.
> +/// \param Loc The kind of diagnostic to emit.
> +/// \param ParenRange Source range enclosing code that should be  
> parenthesized.
> +void Parser::SuggestParentheses(SourceLocation Loc, unsigned DK,
> +                                SourceRange ParenRange) {
> +  if (!ParenRange.getEnd().isFileID()) {
> +    // We can't display the parentheses, so just dig the
> +    // warning/error and return.
> +    Diag(Loc, DK);
> +    return;
> +  }
> +
> +  unsigned Len = Lexer::MeasureTokenLength(ParenRange.getEnd(),
> +                                           PP.getSourceManager());
> +  Diag(Loc, DK)
> +    << CodeInsertionHint(ParenRange.getBegin(), "(")
> +    <<  
> CodeInsertionHint(ParenRange.getEnd().getFileLocWithOffset(Len), ")");

Would it make sense to add a PP.getLocForEndOfToken(SLoc) method or  
something like that?

Thanks again for working on this Doug!

-Chris