[cfe-commits] PATCH: Generate more useful messages for out-of-line definition errors
Kaelyn Uhrain
rikka at google.com
Thu Jul 28 13:53:16 PDT 2011
On Wed, Jul 27, 2011 at 11:20 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jul 25, 2011, at 6:12 PM, Kaelyn Uhrain wrote:
>
> 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.
>
>
> Great! That's a big improvement.
>
> 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"?
>
>
> Yes, definitely. My old "member declaration nearly matches" stuff is really
> lame; your patch is far better. And we can specialize it further for common
> issues (wrong cv qualifiers on the member function, for example).
>
I've expanded the new message to apply to any mismatched parameter including
ones with missing qualifiers. The old message still is used for cases where
the functions don't quite match but have identical parameter lists.
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. ;)
>
>
> Agreed that Fix-Its can be a follow-on patch. This patch already improves
> things quite a bit.
>
> 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.
> <out-of-line-def-error2.diff>
>
>
> + /// Retrieves the base type, e.g. for a "const int*" returns the
> QualType for
> + /// int, but for a "double" returns itself.
> + const QualType getBaseType() const;
>
> I have a hard time calling this the "base type". I'm not sure what name to
> give it, but "base type" sounds too much like inheritance. Perhaps I'm
> confused because...
>
I've replaced QualType::getBaseType with a non-recursive static function in
SemaDecl.cpp (called getCoreType to help it sound less related to
inheritance). If you'd rather keep QualType::getBaseType (possibly renamed),
I committed its removal to my local git tree separately from the rest of the
changes this patch.
> if (!Context.hasSameUnqualifiedType(DeclParamTy.getNonReferenceType(),
> - DefParamTy.getNonReferenceType()))
> - return false;
> + DefParamTy.getNonReferenceType()))
> {
> + if (DeclParamBaseTy == DefParamBaseTy ||
> + (DeclTyName && DeclTyName == DefTyName))
> + Params.push_back(Idx);
> + else
> + return false;
> + }
> }
>
> I don't see why we're bothering with the name checks here. Perhaps the
> presence of isNearlyMatchingFunction() is actually confusing things. I,
> personally, would rather see something more like overload resolution:
> provide a list of possible matches, and for each describe the first problem
> that caused it not to match. Over time, we can hone in the diagnostics (with
> Fix-Its for common issues), rank the candidates and perhaps prune the worst
> of the candidates. isNearlyMatchingFunction() is the wrong abstraction, and
> while your patch makes it better, I think we need to drop the
> "nearly-matching" focus to make progress.
>
I looked at the overload resolution code and from what I could tell the only
real difference between what it does and what isNearlyMatchingFunction does
(not counting the ranking the overload resolution does to try to come up
with a single "best" match) is that the overload resolution handles
mismatched argument types by attempting implicit type conversions. Since
we're trying to match declarations and not a call to a declaration,
parameter type conversions are irrelevant; what is needed is to determine if
the parameters in a given position are similar enough to be what the
programmer may have meant. Part of that determination is to see if the
unqualified non-reference types match, and this all that was originally done
for the parameters. I've added two more pieces to that fuzzy matching:
1) do the underlying (or "core") types match, regardless of '&', '*'. or
'[]', i.e. is the type a double* instead of a double or a const &int[]
instead of an ∫ or
2) do the textual names of the types--sans namespace qualifiers and
such--match, i.e. was "S1" typed when "N2::S1" needed to be typed because
the compiler treats the plain "S1" as "N2::N1::S1" in the definition even
though it resolved a plain "S1" to "N2::S1" in the declaration.
Also, building a list of possible matches and choosing between them would be
independent of the changes to isNearlyMatchingFunction as that function is
just a helper called by DiagnoseInvalidRedeclaration to compare a single
function declaration with the redeclaration being diagnosed.
DiagnoseInvalidRedeclaration handles the set of function declarations with
the same name as the redeclaration being diagnosed; right now
DiagnoseInvalidRedeclaration just prints out notes about the candidates it
finds that are similar enough ("nearly matching") as it finds them. I feel
ranking and further pruning the set of candidates is a refinement for a
later patch, to help move closer to providing fixits for common issues while
also improving the ordering of notes in cases where the invalid
redeclaration is for an overloaded function. This patch is more about
improving the search for likely candidates (and improving the contents of
each displayed note) than about figuring out which of the candidates found
is most likely what the programmer meant. ;)
Cheers,
Kaelyn
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110728/873dfe4f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: out-of-line-def-error3.diff
Type: text/x-diff
Size: 9051 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110728/873dfe4f/attachment.diff>
More information about the cfe-commits
mailing list