<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Kaelyn,<div><br><div><div>On Jul 28, 2011, at 1:53 PM, Kaelyn Uhrain wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_quote">On Wed, Jul 27, 2011 at 11:20 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><br><div><div><div></div><div class="h5"><div>On Jul 25, 2011, at 6:12 PM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite">Doug,<br><br>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. :)<br>
<br><div class="gmail_quote">
On Mon, Jul 25, 2011 at 11:50 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="word-wrap:break-word"><br><div><div><div></div><div><div>On Jul 25, 2011, at 11:38 AM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite"><div class="gmail_quote">On Mon, Jul 25, 2011 at 11:13 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div></div><div><br><div><div>On Jul 22, 2011, at 4:19 PM, Kaelyn Uhrain wrote:</div><br><blockquote type="cite">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.<br>



<br>The patch is also available for review at <a href="http://codereview.appspot.com/4817047" target="_blank">http://codereview.appspot.com/4817047</a><br>

<br>For example, given a file tmp.cpp containing:<br><br>namespace N2 {<br> struct S1;<br><br>  namespace N1 {<br>   struct S2 {<br>     void func(S1*);<br>   };<br><br>   struct S1 {};<br>  }<br>}<br>void N2::N1::S2::func(S1*) {}<br>



<br>Clang currently reports:<br><br>tmp.cpp:12:18: error: out-of-line definition of 'func' does not match any declaration in 'N2::N1::S2'<br>void N2::N1::S2::func(S1*) {}<br>     ~~~~~~~~~~~~^<br><br>Sadly, g++ (version 4.4.3) gives better messages:<br>



<br>tmp.cpp:12: error: prototype for 'void N2::N1::S2::func(N2::N1::S1*)' does not match any in class 'N2::N1::S2'<br>tmp.cpp:6: error: candidate is: void N2::N1::S2::func(N2::S1*)<br><br>With this patch, clang yields:<br>



<br>tmp.cpp:12:18: error: out-of-line definition of 'void func(N2::N1::S1 *)' does not match any<br>      declaration in 'N2::N1::S2'<br>void N2::N1::S2::func(S1*) {}<br>     ~~~~~~~~~~~~^<br>tmp.cpp:6:11: note: member declaration 'void func(N2::S1 *)' nearly matches<br>



     void func(S1*);<br>          ^<br></blockquote></div><div><br></div></div></div>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).</div>


</blockquote><div><br>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.<br>

</div></div></blockquote><div><br></div></div></div>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.,</div>

<div><br></div><div> 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*')</div><div><br></div><div>
Otherwise, we're sending the user hunting for the problem for any function with more than one parameter.</div>
</div></blockquote><div><br>Yeah, the form of the diagnostic was the part I was most unsure about. With my new patch, the original example will now yield:<br><br> tmp.cpp:12:18: error: out-of-line definition of 'func' does not match any declaration in 'N2::N1::S2'<br>

void N2::N1::S2::func(S1*) {}<br>     ~~~~~~~~~~~~^<br>tmp.cpp:6:16: note: Type of 1st parameter of member declaration does not match definition: 'N2::S1 *'<br>      vs 'N2::N1::S1 *'<br>     void func(S1*);<br>

               ^<br>1 error generated.<br><br>Mismatched built-in types such as double* instead of double now print the above note too.<br></div></div></blockquote><div><br></div></div></div>Great! That's a big improvement.</div>
<div><div class="im"><br><blockquote type="cite"><div class="gmail_quote"><div>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"?<br>
</div></div></blockquote><div><br></div></div><div>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).</div>
</div></div></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><div class="im"><blockquote type="cite">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. ;)<br>
</blockquote><div><br></div></div>Agreed that Fix-Its can be a follow-on patch. This patch already improves things quite a bit.</div><div><br><blockquote type="cite"><div class="im">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.<br>

</div><span><out-of-line-def-error2.diff></span></blockquote></div><br><div><div>+  /// Retrieves the base type, e.g. for a "const int*" returns the QualType for</div><div>+  /// int, but for a "double" returns itself.</div>
<div>+  const QualType getBaseType() const;</div></div><div><br></div><div>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...</div>
</div></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><div style="word-wrap: break-word;"><div><br></div><div><div>     if (!Context.hasSameUnqualifiedType(DeclParamTy.getNonReferenceType(),</div>
<div>-                                        DefParamTy.getNonReferenceType()))</div><div>-      return false;</div><div>+                                        DefParamTy.getNonReferenceType())) {</div><div>+      if (DeclParamBaseTy == DefParamBaseTy ||</div>
<div>+          (DeclTyName && DeclTyName == DefTyName))</div><div>+        Params.push_back(Idx);</div><div>+      else</div><div>+        return false;</div><div>+    }</div><div>   }</div></div><div><br></div><div>
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.</div>
</div></blockquote><div><br>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:<br>
   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 &int; or<br>   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.<br>
<br>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. ;)<br></div></div></blockquote></div><br></div><div>I think this is a great step forward. I have only one trivial request before you commit it:</div><div><br></div><div><div>ndex 239bd45..f5dd1fb 100644</div><div>--- a/include/clang/Basic/DiagnosticSemaKinds.td</div><div>+++ b/include/clang/Basic/DiagnosticSemaKinds.td</div><div>@@ -2965,6 +2965,9 @@ def warn_member_extra_qualification : Warning<</div><div> def err_member_qualification : Error<</div><div>   "non-friend class member %0 cannot have a qualified name">;  </div><div> def note_member_def_close_match : Note<"member declaration nearly matches">;</div><div>+def note_member_def_close_param_match : Note<</div><div>+  "Type of %ordinal0 parameter of member declaration does not match "</div><div>+  "definition: %1 vs %2">;</div></div><div><br></div><div>"Type" should not be capitalized. I also see that it's more common for us to use "(%1 vs %2)" rather than ": %1 vs %2". </div><div><br></div><div>And we're totally inconsistent about "vs" vs. "vs." Or is it "vs." vs "vs" that we're inconsistent about?</div><div><br></div><div>Thanks for working on this!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre">        </span>- Doug</div></body></html>