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

[Chris Lattner]

On Nov 18, 2008, at 8:13 AM, Douglas Gregor wrote:
>> 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.

Sure, this is just a couple steps in the right direction, it's not  
done yet :)

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

Sure, we have stuff like this:

   if (Diags.getDiagnosticLevel(DiagID) != Diagnostic::Ignored)
     ...

but...

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

This commit was just one step along the way.  Currently diags still  
traffic in std::strings.  I'd like to change DiagnosticInfo to use an  
array of unions of "stuff", some of which may be strings, some may be  
types, some may be integers etc.  If the diagnostic is never printed,  
the formatting would just never happen.

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

Me neither.  I'll try to come up with a better solution, for the  
reasons you raise and those that Sebastian raises.  The tricky part  
has to do with when DiagnosticInfo has more than just strings in it.

> I know that in the use cases
> you're thinking of, the whole diagnostic will happen on one line:
>
> 	Diag(location, id) << foo.getAsString() << ... ;

Yep.

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

I agree it sucks, but you could explicitly bind any temporaries to  
variables...

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

Yes, we also have "expected ')'"  ... to match this "(".  It has two  
carets and needs to be formatted as two locations/diags, but they are  
conceptually one.

I'd like to solve this problem in the future as well, but not right  
now :)

> 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 :)

One way to handle this is to introduce a new diag kind, "CONTINUATION"  
or something like that.  The IDE would just treat a CONTINUATION diag  
as part of the previous one if it wanted to.

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

This will be fixed in the future.

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

This too :)

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

Yep exactly.

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

Totally agreed, will do as follow on patches.

Thanks Doug!

-Chris