arphaman added inline comments.
================
Comment at: test/Misc/diag-template-diffing.cpp:27
// CHECK-ELIDE-NOTREE: no matching function for call to 'f'
-// CHECK-ELIDE-NOTREE: candidate function not viable: no known conversion from
'vector<std::basic_string>' to 'vector<versa_string>' for 1st argument
+// CHECK-ELIDE-NOTREE: candidate function not viable: no known conversion from
'vector<std::string>' to 'vector<string>' for 1st argument
// CHECK-NOELIDE-NOTREE: no matching function for call to 'f'
----------------
redm123 wrote:
> redm123 wrote:
> > arphaman wrote:
> > > I think the majority of test changes make sense, we are just adding
> > > qualifiers to the typedefs in the diagnostics. I'm curious about this one
> > > though, as we are essentially replacing the old diagnostic note `no known
> > > conversion from 'vector<std::basic_string>' to 'vector<versa_string>' for
> > > 1st argument` by the new note `no known conversion from
> > > 'vector<std::string>' to 'vector<string>' for 1st argument`. Is one
> > > better than the other? It seems that GCC prefers the former, while ICC
> > > the latter. Does it even matter which one we have?
> > Right, wanted to add a note... forgot about that. This "fix" here is
> > probably entirely nonsense. I guess I broke some other feature here. I was
> > hoping for some input from the review ;) As I see it it's supposed to print
> > the canonical type in case both types would be printed the same, but
> > apparently the comparison breaks. The question where this happens... I'll
> > try to dig through the history and see if I find something.
> So the problematic place seems to be in lib/AST/ASTDiagnostic.cpp:
> PrintTypeNames() which compares the printed type names, and as this
> versa_string has no namespace it breaks... We could restore the old behavior
> for these tests with something like:
>
> Index: lib/AST/ASTDiagnostic.cpp
> ===================================================================
> --- lib/AST/ASTDiagnostic.cpp (revision 294732)
> +++ lib/AST/ASTDiagnostic.cpp (working copy)
> @@ -1612,10 +1612,12 @@
> return;
> }
>
> + PrintingPolicy ComparePolicy = Policy;
> + ComparePolicy.SuppressScope = true;
> std::string FromTypeStr = FromType.isNull() ? "(no argument)"
> - :
> FromType.getAsString(Policy);
> + :
> FromType.getAsString(ComparePolicy);
> std::string ToTypeStr = ToType.isNull() ? "(no argument)"
> - : ToType.getAsString(Policy);
> + :
> ToType.getAsString(ComparePolicy);
> // Switch to canonical typename if it is better.
> // TODO: merge this with other aka printing above.
> if (FromTypeStr == ToTypeStr) {
>
> But as this disables namespace scopes also for normal types (not just
> typedefs), things now break further down...
>
> So what would be a proper fix...?
>
> If I see it right this test case only breaks in the first place because it
> uses typedefs and no namespaces were printed for typedefs up to now. I.e. you
> get "no known conversion from 'vector<string>' to 'vector<string>'", as the
> comment says, which is indeed somewhat ambiguous. With my patch you now get
> "no known conversion from 'vector<std::string>' to 'vector<string>'" ...
> better.
>
> Interestingly if you have the same example with no typedefs, but just "class
> string;", you get the same error, as in case of non-typedefs the namespaces
> were always printed. In this case it seems to be sufficiently non-ambiguous.
>
> **So**: As with my patch we seem to achive the same result, we could also
> remove this entire test, because this case can't happen anymore.
> **Further so**: Actually we might also remove the entire check and fallback
> to canonical types in PrintTypeNames(), as this also can't happen anymore...
>
> At least AFACIS...
>
> Thoughts?
>
>
Sorry about the delay,
You're right about `PrintTypeNames`. We shouldn't the suppress scope though, at
least not for the types that have different string representation. We could try
to suppress it, and see if the types have identical string representation, and
if not, then we can get the un-supressed representation.
I'm not sure if that's really worth doing though, and IMHO the the patched
version of the diagnostic seems fine. It's still visible to the user that
`std::string` is different to `string`, which means that the original issue is
addressed. I think that if no-one else has any objections then this change can
be approved (I'll wait a couple of days and then will approve).
This particular test is still worth keeping around I think.
https://reviews.llvm.org/D29944
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
