[cfe-commits] r59502 - in /cfe/trunk: Driver/ include/clang/Analysis/ include/clang/Analysis/PathSensitive/ include/clang/Basic/ include/clang/Driver/ lib/Analysis/ lib/Basic/ lib/CodeGen/ lib/Driver/ lib/Lex/ lib/Parse/ lib/Sema/

[Douglas Gregor]

On Nov 18, 2008, at 2:04 AM, Chris Lattner wrote:

> Author: lattner
> Date: Tue Nov 18 01:04:44 2008
> New Revision: 59502
>
> URL: http://llvm.org/viewvc/llvm-project?rev=59502&view=rev
> Log:
> This reworks some of the Diagnostic interfaces a bit to change how  
> diagnostics
> are formed.  In particular, a diagnostic with all its strings and  
> ranges is now
> packaged up and sent to DiagnosticClients as a DiagnosticInfo  
> instead of as a
> ton of random stuff.  This has the benefit of simplifying the  
> interface, making
> it more extensible, and allowing us to do more checking for things  
> like access
> past the end of the various arrays passed in.

Cool, thanks for tackling this.

> We will soon be able to just do:
>
>  Diag(BuiltinLoc, diag::err_overload_no_match)
>      << typeNames << SourceRange(BuiltinLoc, RParenLoc));
>
> This scales better to support arbitrary types being passed in (not  
> just
> strings) in a type-safe way.  Go operator overloading?!

Looks good.

> +class DiagnosticInfo {
> +  Diagnostic *DiagObj;
> +  FullSourceLoc Loc;
> +  unsigned DiagID;
> +  void operator=(const DiagnosticInfo&); // DO NOT IMPLEMENT
> +public:
> +  DiagnosticInfo(Diagnostic *diagObj, FullSourceLoc loc, unsigned  
> diagID) :
> +    DiagObj(diagObj), Loc(loc), DiagID(diagID) {
> +    assert(DiagObj->NumDiagArgs == -1 &&
> +           "Multiple diagnostics in flight at once!");
> +    DiagObj->NumDiagArgs = DiagObj->NumDiagRanges = 0;
> +  }
> +
> +  /// Copy constructor.  When copied, this "takes" the diagnostic  
> info from the
> +  /// input and neuters it.
> +  DiagnosticInfo(DiagnosticInfo &D) {
> +    DiagObj = D.DiagObj;
> +    Loc = D.Loc;
> +    DiagID = D.DiagID;
> +    D.DiagObj = 0;
> +  }
> +
> +  /// Destructor - The dtor emits the diagnostic.
> +  ~DiagnosticInfo() {
> +    // If DiagObj is null, then its soul was stolen by the copy ctor.
> +    if (!DiagObj) return;
> +
> +    DiagObj->ProcessDiag(*this);
> +
> +    // This diagnostic is no longer in flight.
> +    DiagObj->NumDiagArgs = -1;
> +  }
> +
> +  const Diagnostic *getDiags() const { return DiagObj; }
> +  unsigned getID() const { return DiagID; }
> +  const FullSourceLoc &getLocation() const { return Loc; }
>

Is there an easy way to determine whether this diagnostic has been  
suppressed, such as a method

	bool isSuppressed() const;

?

If so, then the operator<<'s for DiagnosticInfo could first check  
isSuppressed(), and abort early if the diagnostic is suppressed. That  
way, we never pay for formatting strings (e.g., for a type name or  
expression) or copying them (see my comment below).

> +  DiagnosticInfo &operator<<(const std::string &S) {
> +    assert((unsigned)DiagObj->NumDiagArgs <
> +           sizeof(DiagObj->DiagArguments)/sizeof(DiagObj- 
> >DiagArguments[0]) &&
> +           "Too many arguments to diagnostic!");
> +    DiagObj->DiagArguments[DiagObj->NumDiagArgs++] = &S;
> +    return *this;
> +  }

Oh, I really don't like the &S here. I know that in the use cases  
you're thinking of, the whole diagnostic will happen on one line:

	Diag(location, id) << foo.getAsString() << ... ;

However, it seems plausible to me that we might also want to grab a  
name for the result of Diag() and then add more information to it  
afterward:

	DiagnosticInfo D = Diag(location, id) << foo.getAsString << .. ;
	// add more stuff to D
	// on destruction of D, we crash (probably)

The case I'm thinking about most is for reporting the results of an  
overload resolution failure, where we currently output several related  
diagnostics. The first says what the problem is:

overload-call.cpp:184:3: error: call to 'quals_rank3' is ambiguous;  
candidates are:
   quals_rank3(p); // expected-error {{call to 'quals_rank3' is  
ambiguous; candidates are:}}
   ^~~~~~~~~~~

then we have some follow-on "notes"  that show the candidates:

overload-call.cpp:168:6: note: candidate function
void quals_rank3(int const *); // expected-note{{candidate function}}
      ^
overload-call.cpp:169:6: note: candidate function
void quals_rank3(int volatile *); // expected-note{{candidate function}}
      ^

It seems wrong (to me!) to have these be separate errors; rather,  
these notes should be formatted as part of the original message,  
because it's all (conceptually) one error, and an IDE client would  
want to treat it as one error (perhaps with some interactive  
exploration of the candidate functions). There's a lot of info we  
could put into that DiagnosticInfo object, but it's tougher if we  
can't name it :)

There are other reasons not to like the &S. For example, if we're  
outputting a QualType named "Ty" through a diagnostic, e.g.,

	Diag(Lok, ID) << Ty;

we're going to need to create a temporary std::string to store the  
formatted type name, but where do we store that temporary so that  
DiagnosticInfo can hold on to it?

Besides, if we have isSuppressed(), we'll only be doing string copying  
and formatting when the diagnostic is actually going to be displayed,  
so we're not as worried about performance once we're emitting the  
diagnostic.

> +  DiagnosticInfo &operator<<(const SourceRange &R) {
> +    assert((unsigned)DiagObj->NumDiagArgs <
> +           sizeof(DiagObj->DiagRanges)/sizeof(DiagObj- 
> >DiagRanges[0]) &&
> +           "Too many arguments to diagnostic!");
> +    DiagObj->DiagRanges[DiagObj->NumDiagRanges++] = &R;
> +    return *this;
> +  }
> +
> };

These operator<<'s should be free functions, because we will certainly  
have other operator<<'s for DiagnosticInfo elsewhere in the compiler  
that must be free functions, e.g.,

	DiagnosticInfo& operator<<(DiagnosticInfo& DI, QualType Ty); // in  
clang/AST/Type.h

and we don't want non-member and member operator<<'s overloaded  
because the available conversions on the first argument differ, and  
that causes overloading weirdness.

	- Doug