[cfe-commits] PATCH: Generate more useful messages for out-of-line definition errors
Kaelyn Uhrain
rikka at google.com
Mon Jul 25 18:12:02 PDT 2011
Doug,
Thanks for the review. I've attached an updated patch (and uploaded the
patchset to Rietveld) incorporating your feedback. The new patch is smaller
and touches fewer files. :)
On Mon, Jul 25, 2011 at 11:50 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jul 25, 2011, at 11:38 AM, Kaelyn Uhrain wrote:
>
> On Mon, Jul 25, 2011 at 11:13 AM, Douglas Gregor <dgregor at apple.com>wrote:
>
>>
>> On Jul 22, 2011, at 4:19 PM, Kaelyn Uhrain wrote:
>>
>> Here's a patch to improve the messages for out-of-line definition errors
>> by including the function's signature along with its name, and in the
>> candidate function notes including the candidates name and signature as
>> namespace qualifiers aren't always apparent at the line of code with the
>> declaration.
>>
>> The patch is also available for review at
>> http://codereview.appspot.com/4817047
>>
>> For example, given a file tmp.cpp containing:
>>
>> namespace N2 {
>> struct S1;
>>
>> namespace N1 {
>> struct S2 {
>> void func(S1*);
>> };
>>
>> struct S1 {};
>> }
>> }
>> void N2::N1::S2::func(S1*) {}
>>
>> Clang currently reports:
>>
>> tmp.cpp:12:18: error: out-of-line definition of 'func' does not match any
>> declaration in 'N2::N1::S2'
>> void N2::N1::S2::func(S1*) {}
>> ~~~~~~~~~~~~^
>>
>> Sadly, g++ (version 4.4.3) gives better messages:
>>
>> tmp.cpp:12: error: prototype for 'void N2::N1::S2::func(N2::N1::S1*)' does
>> not match any in class 'N2::N1::S2'
>> tmp.cpp:6: error: candidate is: void N2::N1::S2::func(N2::S1*)
>>
>> With this patch, clang yields:
>>
>> tmp.cpp:12:18: error: out-of-line definition of 'void func(N2::N1::S1 *)'
>> does not match any
>> declaration in 'N2::N1::S2'
>> void N2::N1::S2::func(S1*) {}
>> ~~~~~~~~~~~~^
>> tmp.cpp:6:11: note: member declaration 'void func(N2::S1 *)' nearly
>> matches
>> void func(S1*);
>> ^
>>
>>
>> I'm not convinced we want to go in this direction. Or general principle
>> for Clang diagnostics has been "show, don't tell", and we'd much rather
>> point out specifically where the mismatch occurs than print out the entire
>> type of the function and force the user to walk through the parameters to
>> find the mismatch. This approach also allows us to add Fix-Its when the
>> mismatch is due to something easily fixed (e.g., missing cv-qualifiers).
>>
>
> Clang currently only suggests match candidates if there is a slight
> mismatch such as missing cv-qualifers, but all of the underlying
> (unqualified) types have to match. My patch just expands that to match cases
> where the types are different but have the same name--where there are
> missing/implicit namespace qualifers, which currently will give a totally
> useless and very baffling message to the user. In the example I gave
> previously, it says N2::N1::S2::func(S1*) is unmatched but the user will
> look in the code and see func(S1*) declared in struct N2::N1::S2 with no
> idea why clang wasn't accepting the message.
>
>
> I'm objecting more to the form of the diagnostic than anything else.
> Printing the full function signature is going to make the diagnostic text
> very, very long. I'd rather have the diagnostic focus in on the specific
> parameter that mismatched, e.g.,
>
> note: first parameter of member declaration has a type incompatible with
> corresponding parameter in the out-of-line definition ('N2::S1 *' vs.
> 'N2::N1::S1*')
>
> Otherwise, we're sending the user hunting for the problem for any function
> with more than one parameter.
>
Yeah, the form of the diagnostic was the part I was most unsure about. With
my new patch, the original example will now yield:
tmp.cpp:12:18: error: out-of-line definition of 'func' does not match any
declaration in 'N2::N1::S2'
void N2::N1::S2::func(S1*) {}
~~~~~~~~~~~~^
tmp.cpp:6:16: note: Type of 1st parameter of member declaration does not
match definition: 'N2::S1 *'
vs 'N2::N1::S1 *'
void func(S1*);
^
1 error generated.
Mismatched built-in types such as double* instead of double now print the
above note too.
The original cases where clang would output a "member declaration nearly
matches" note (where ASTContext::hasSameUnqualifiedType returns true but the
qualified types differ, e.g. double vs double&) are now back to how they
currently are in svn--shall I replace those cases in
isNearlyMatchingFunction to also use the new note instead of "member
declaration nearly matches"?
>
>
>> Overload-resolution diagnostics are a good model for what we want here. We
>> don't just print out the types of the arguments and the types of the
>> candidate functions; instead, we only point out the specific problem that
>> caused us to reject each candidate, so users can focus on that
>> parameter/argument pair.
>>
>
>
> I actually started to save off which parameters were causing the mismatch
> so they could be reported, but I could not come up with a way to mark it
> without being too ridiculously verbose (especially if more than one param
> was a mismatch).
>
>
> I suggest just reporting the first mismatch.
>
Done!
>
> And this is also a case where choosing an appropriate fixit would be
> problematic since e.g. in the example from my previous email it is unclear
> whether the definition of func(S1*) should be changed to func(::S1*) or if
> the first declaration of struct S1 accidentally done at the wrong namespace
> scope. I'll take a look at the overload-resolution diagnostic to see how it
> handles this kind of mismatch.
>
>
> I don't expect a Fix-It specifically for this weird name mismatch, although
> it would be really cool. I'm saying that, if we isolate the specific
> parameter(s) that fail, we can easily add logic to fix common problems,
> e.g.,
>
> struct X {
> void f(const std::string &);
> };
>
> void X::f(std::string) { // Fix-It could suggest the "const &" here easily
> enough
> }
>
>
Yeah, adding fixits in the cases that makes sense would be really cool but
for a separate patch. My comments regarding fixits was more just me dumping
what my thinking was regarding the diagnostic message. ;)
Cheers,
Kaelyn
P.S. It just occurred to me that capturing the parameters for which
hasSameUnqualifiedType is true and the qualified types are different (like
my patch already does for cases where hasSameUnqualifiedType is false but
the base type names without namespace qualifiers are the same) would
simplify adding fixits for those cases.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110725/3b3679cb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: out-of-line-def-error2.diff
Type: text/x-diff
Size: 8090 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110725/3b3679cb/attachment.diff>
More information about the cfe-commits
mailing list