This is an archive of the discontinued LLVM Phabricator instance.

libclang: Print namespaces for typedefs and type aliases
ClosedPublic

Authored by redm123 on Feb 14 2017, 8:11 AM.

Details

Summary

Printing typedefs or type aliases using clang_getTypeSpelling() is missing the namespace they are defined in. This is in contrast to other types that always yield the full typename including namespaces.

Diff Detail

Repository
rL LLVM

Event Timeline

redm123 created this revision.Feb 14 2017, 8:11 AM

I'm quite surprised by the fact that we have to change so many diagnostics in tests. Do these diagnostics have the full qualifiers for errors that occur with record types instead of typedefs?

Do these diagnostics have the full qualifiers for errors that occur with record types instead of typedefs?

Not sure what you mean by that, but as far as I can see all the changed errors refer to typedefs/aliases. Normal records types should not be affected.

arphaman added inline comments.Feb 17 2017, 4:06 PM
test/Misc/diag-template-diffing.cpp
27 ↗(On Diff #88376)

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?

redm123 added inline comments.Feb 20 2017, 6:27 AM
test/Misc/diag-template-diffing.cpp
27 ↗(On Diff #88376)

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.

redm123 added inline comments.Feb 20 2017, 9:56 AM
test/Misc/diag-template-diffing.cpp
27 ↗(On Diff #88376)

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?

Any idea how to continue with that? Should I add another reviewer? Whom?

arphaman added inline comments.Mar 2 2017, 10:54 AM
test/Misc/diag-template-diffing.cpp
27 ↗(On Diff #88376)

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.

arphaman accepted this revision.Mar 10 2017, 3:00 AM

LGTM, thanks for the fix. Do you need someone to commit it for you?

This revision is now accepted and ready to land.Mar 10 2017, 3:00 AM

I guess so. I don't think I'm allowed to commit. So I would appreciate it.

Which release version would that be in? 4.1 I guess?

Thanks for your help!

I guess so. I don't think I'm allowed to commit. So I would appreciate it.

Sure, I will commit it right now.

Which release version would that be in? 4.1 I guess?

It will be in the next major release: 5.0 (After 4.0 LLVM switched to a different versioning system where major released will now go from 4.0 to 5.0, etc. instead of 4.0 to 4.1, etc.).

Thanks for your help!

This revision was automatically updated to reflect the committed changes.