Page MenuHomePhabricator

Support fully-qualified names for all QualTypes
ClosedPublic

Authored by saugustine on Jan 4 2016, 11:24 AM.

Details

Reviewers
rsmith
Summary

Calculate the fully qualified name of any QualType, including
recursively expanding template parameters, elaborated types and
other syntactice sugar.

This patch is adapted from code in the Cling project by Vassil
Vassilev<vasil.georgiev.vasilev@cern.ch> and Philippe Canal
<pcanal@fnal.gov>.

Diff Detail

Event Timeline

saugustine updated this revision to Diff 43903.Jan 4 2016, 11:24 AM
saugustine retitled this revision from to Support fully-qualified names for all QualTypes.
saugustine updated this object.

(I'm not doing a full review, I just happened to notice a couple things when skimming.)

lib/Tooling/Core/QualTypeNames.cpp
251

s/dyn_cast_or_null/dyn_cast/

You've already checked !TypePtr above, so it can't be null here.

307

s/qualifiy/qualify/

rsmith added inline comments.Jan 4 2016, 4:07 PM
include/clang/Tooling/Core/QualTypeNames.h
33

Please ensure there's a space between each /// and the content.

33–34

What do you mean by "fully qualified template arguments" here? Let me give you some examples:

namespace A {
  struct X {};
}
using A::X;
namespace B {
  using std::tuple;
  typedef typle<X> TX;
  TX t;
  struct A { typedef int X; };
}

What is the fully-qualified name of B::t's type? Is it B::TX or std::tuple<A::X> or std::tuple<::A::X>? Note that if you want to redeclare t from within namespace B, std::tuple<A::X> will name the wrong type.

Why does this only affect template arguments? Its name suggests it should affect the type as a whole (for instance, in the above case it should produce std::tuple<...>, not tuple<...>).

Generally, I think this interface needs to specify where the produced names can be used as a name for the specified type, otherwise I don't see how it can ever be reliable. For instance:

"Generates a name for a type that can be used to name the same type if used at the end of the current translation unit." (eg, B::TX or std::tuple<X>)

or:

"Generates a maximally-explicit name for a type that can be used in any context where all the relevant components have been declared. In such a context, this name will either name the intended type or produce an ambiguity error." (eg, ::std::tuple<::A::X>)

You should also specify what happens when it's not possible to generate such a name. For instance, given:

void f() {
  struct X {} x;
}

... there's no way to name the type of x from outside f (which makes certain refactoring operations impossible unless you also move the definition of struct X).

I think the ideal interface here would allow you to specify a location where you wish to insert the name, and would produce a "best possible" name for that type for that location, avoiding adding explicit qualification / desugaring wherever possible, but the interface should at least take the context-sensitivity of names into account.

38

Remove the clang::s; you're already in namespace clang.

50–80

Is it useful to expose the intermediary functionality to generate fully-qualified QualTypes and NestedNameSpecifiers rather than only exposing functionality to generate strings? What are the intended uses of these functions?

lib/Tooling/Core/QualTypeNames.cpp
342–344

This will not work in C, or if the type name is normally shadowed by a variable or function. You sometimes need to keep the keyword.

saugustine updated this revision to Diff 44245.Jan 7 2016, 12:07 PM
saugustine marked 4 inline comments as done.
  • Update docs. Handle keywords and anonymous namespaces.

Thanks for the reviews. Please take another look when you get a chance.

include/clang/Tooling/Core/QualTypeNames.h
33–34

My use case is to take a function signature, and communicate to a developer one way to declare the variables they need to call the function.

It does expand entire qualtypes, not just template parameters. (I've updated that description.)

Given the use case, "at the end of the translation unit" is the closest description of where these names would be valid, with the exception that this code avoids relying on any "using" declaration. "using foo::bar; void bat(bar b);", this code would describe foo's parameter as type foo::bar, rather than plain "bar", even though plain "bar" would work at the end of the translation unit.

I have updated the file header's comment to reflect all this, and added a couple of test cases to prove to myself that it does what I have documented. Along the way I have found a couple of places to explicitly mark where one would do things differently if one wanted to change this behavior.

The "ideal interface" idea is a good--and very cool--one, but my use case doesn't call for it.

50–80

I believe the Cling project (from which this code is adapted) uses all those functions internally. So I expect at least one client for those functions.

Let me know if you think hiding this interface is still the right thing.

lib/Tooling/Core/QualTypeNames.cpp
342–344

I've rewritten this code a bit, and this problem is solved near the end of this function. (This section itself is now mostly gone.)

rsmith edited edge metadata.Jan 13 2016, 12:47 PM

I think this functionality can be provided more simply by adding another flag to PrintingPolicy to request fully-qualified names always be produced. It already usually provides fully-qualified names; the only notable exception I can see is that if a qualifier was already provided, it uses that instead (in particular, there is a special case for printing ElaboratedTypes that uses the qualifier as written).

include/clang/Tooling/Core/QualTypeNames.h
56

refering -> referring

81

Missing /// on this line.

108

&, not & , please.

119–140

It would probably be better to have a single function to create a NestedNameSpecifier from a TypeDecl rather than splitting up these two cases.

134

TypedefDecl -> TypedefNameDecl

lib/Tooling/Core/QualTypeNames.cpp
32

NameSpecifier -> NestedNameSpecifier

52

ArgTDecl could be null here (for instance, if the TemplateName is a dependent template name).

88

What do you mean by 'Local' here?

90

I don't think this comment is accurate, just delete it?

99–100

Comment should start with a capital letter and end in a period.

106

This comment is inaccurate; you're not doing desugaring. (The name of DesArgs is also inappropriate; QualArgs or FQArgs or similar would better express the intent.)

108–111

Don't you need to form a fully-qualified template name here too?

151–154

This won't do the right thing if you reach an extern "C" or extern "C++" context. You can use getRedeclContext to skip those.

155

This will crash if you reached the TranslationUnitDecl or NS otherwise became null.

156

This is the wrong result if you reach an anonymous namespace nested within a named namespace.

167

Make this a function doc comment. And likewise for the similar cases below.

174–176

Why do you desugar namespace aliases here?

178–179

This indicates that the NestedNameSpecifier is dependent. Leaving this component alone seems like the right thing to do, but you should still fully-qualify the prefix.

187

Do not use dyn_cast on a Type*, it won't do what you want with type sugar. Use Type->getAs<TagType>() instead.

210–213

Call getRedeclContext here to skip extern "C" and the like.

214

If the outer context is an anonymous namespace, you should skip over it; it might be within another namespace.

215–235

I don't think this is the right way to handle this situation (it would be better to try to build the nested name specifier for the type, and fail if you can't build one that would name the right context). However, I don't see why you need to handle this at all -- it doesn't make sense to call this function with an instantiation-dependent QualType, as the notional "at the end of the translation unit" context has no template parameters in scope. You should probably detect and bail out on instantiation-dependent types earlier in the process.

241–246

The context might also be the translation unit here.

277

This is wrong in the same way as the similar code above.

301

This seems to miss out a lot of cases.

307

You don't need llvm:: on isa.

405

This call ignores a lot of the things you messed with above. It seems like only a couple of the modifications you make to the type actually affect how it's printed at all.

saugustine updated this revision to Diff 46680.Feb 2 2016, 11:14 AM
saugustine marked 19 inline comments as done.
saugustine edited edge metadata.
  • Update docs. Handle keywords and anonymous namespaces.
  • Address code review issues. Cleanup many

Richard,

Please take another look when you get a chance. Thanks.

saugustine updated this revision to Diff 47267.Feb 8 2016, 3:34 PM
  • Privatize all functions but getFullyQualifiedName.
rsmith accepted this revision.Feb 8 2016, 3:50 PM
rsmith edited edge metadata.

OK, let's go ahead with this approach for now; we can investigate replacing the implementation with a PrintingPolicy flag later.

lib/Tooling/Core/QualTypeNames.cpp
309–313

You don't need llvm:: on dyn_cast. It's more idiomatic to use const auto *TDT = dyn_cast<TypedefType>(TypePtr) rather than repeating the type on both sides of the =.

311–317

Maybe drop the braces around this to form an if/else if chain.

This revision is now accepted and ready to land.Feb 8 2016, 3:50 PM
saugustine updated this revision to Diff 47343.Feb 9 2016, 11:02 AM
saugustine edited edge metadata.
  • Cleanup dyn_cast usage inside QualTypeNames.

I've updated the change.

Would you mind checking it in? I'll look into switching the implementations
shortly.

rsmith closed this revision.Feb 9 2016, 1:08 PM

In r260278.