[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/

[Douglas Gregor]

On Feb 26, 2009, at 10:22 PM, Chris Lattner wrote:

>
> 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!

Thanks for the review!

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

Will do.

>
>> +++ 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"?
Yes, thanks.

>
>>
>> +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.

Okay.

>> =============================================
>> --- 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.

Sure.

>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- 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.

It's a copy of a 32-bit integer... but I'll check.

>
>> +++ 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.

Done.

>
>> +++ 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?

Yes, because it's a parsing issue, not a semantic issue. The '>>'  
token is treated differently in a template argument list in C++98 vs. C 
++0x, and the parser encodes this behavior in getBinOpPrecedence.  
However, we don't have the information to diagnose this language  
inconsistency until we know that we've parsed the '>>' as an operator  
and have both operands of the expression.

>
>> +++ 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?

Yes, it would. Done!

	- Doug