[PATCH] D20040: Treat qualifiers on elaborated types for qualtypenames appropriately.

[Sterling Augustine via cfe-commits]

saugustine added a comment.

Thanks for the reviews. I believe I have addressed all issues. Take another look when you get the chance.


================
Comment at: lib/Tooling/Core/QualTypeNames.cpp:401-403
@@ -397,6 +400,5 @@
   // move the qualifiers on the outer type (avoid 'std::const string'!)
-  if (Prefix) {
-    PrefixQualifiers = QT.getLocalQualifiers();
+  if (Prefix || Keyword != ETK_None) {
     QT = QualType(QT.getTypePtr(), 0);
   }
 
----------------
rsmith wrote:
> I find the way this code ensures that we preserve the qualifiers to be a little subtle. It's not obvious to me that this does the right thing for a case like
> 
>     struct X;
>     void f(const ::X x) {}
> 
> ... where we have an `ElaboratedType` with no keyword, and for which we will generate an empty `Prefix` -- it looks like we would lose the `const` on line 392 and never add it back.
> 
> Can you remove and re-add the qualifiers unconditionally? (That is, move this removal of qualifiers from `QT` to after line 389, and move line 419 outside the `if`.) I think that'll make the logic clearer.
We can remove and put them back unconditionally, but we still need to condition getting a new elaborated type, because an assertion prevents creating an elaborated type without a prefix of an empty keyword.

I'm not sure what you expect on the new example, but I have added it as a test case (as a pointer because the struct is incomplete). The result we produce in this case is "const X *", which I think is correct.

================
Comment at: unittests/Tooling/QualTypeNamesTest.cpp:91
@@ -90,2 +90,3 @@
   Visitor.ExpectedQualTypeNames["AliasTypeVal"] = "A::B::C::InnerAlias<int>";
+  Visitor.ExpectedQualTypeNames["CheckM"] = "const A::B::Class0 *";
   Visitor.runOver(
----------------
dblaikie wrote:
> What does this produce without your change? (what's the change causing to happen?)
Without the change, we get "A::B::Class0 *"  with no "const".


http://reviews.llvm.org/D20040