Page MenuHomePhabricator

CompilerType: Add ability to retrieve an integral template argument
ClosedPublic

Authored by labath on Nov 9 2017, 7:18 AM.

Details

Summary

Despite it's name, GetTemplateArgument was only really working for Type
template arguments. This adds the ability to retrieve integral arguments
as well (which I've needed for the std::bitset data formatter).

I've done this by splitting the function into three pieces. The idea is
that one first calls GetTemplateArgumentKind (first function) to
determine the what kind of a parameter this is. Based on that, one can
then use specialized functions to retrieve the correct value. Currently,
I only implement two of these: GetTypeTemplateArgument and
GetIntegralTemplateArgument.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Nov 9 2017, 7:18 AM
labath added inline comments.Nov 9 2017, 7:24 AM
source/API/SBType.cpp
422 ↗(On Diff #122247)

I should point out that this changes the behaviour of SBType::GetTemplateArgumentType slightly.

Previously, for an integral template argument (e.g. foo<47>), it returned the *type* of the integer argument (i.e. int), where as now it will return nothing.

I think this is the behavior that makes most sense for the underlying API, but if you're worried about compatibility, I can add a special case here (I am not worried as I don't think there is anything reasonable the user could have done with the returned value anyway).

clayborg requested changes to this revision.Nov 9 2017, 9:38 AM

Just a few changes.

  • I would like the see the SBType returned for the integer template types as it is what I would expect to happen.
  • we should add default implementations for stuff in TypeSystem and not require all languages to override everything. I know this isn't the way things were done in the past, but we should fix that to avoid bloat in all TypeSystem subclasses.
include/lldb/Symbol/GoASTContext.h
318–334 ↗(On Diff #122247)

I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?

include/lldb/Symbol/JavaASTContext.h
206–220 ↗(On Diff #122247)

I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?

include/lldb/Symbol/OCamlASTContext.h
234–248 ↗(On Diff #122247)

I know this isn't how things were done up to now, but we really should move these default implementations in the original class and not require everyone to override with a version that does nothing. Can we not make these pure virtual in the original class?

include/lldb/Symbol/TypeSystem.h
354–360 ↗(On Diff #122247)

Put default implementations here, and don't modify the Go, Java and OCaml ASTs?

source/API/SBType.cpp
422 ↗(On Diff #122247)

I would like this to return the type here as I believe that makes the most sense. Why would we not return the correct type of the template argument for integers?

This revision now requires changes to proceed.Nov 9 2017, 9:38 AM
clayborg added inline comments.Nov 9 2017, 9:49 AM
include/lldb/Symbol/TypeSystem.h
356–360 ↗(On Diff #122247)

Why not make three functions:

  • get kind by index
  • get type by index
  • get integer by index

The "get integer by index" would only work if the kind was eTemplateArgumentKindIntegral? Not sure why we have the pair getter?

Just a few changes.

  • I would like the see the SBType returned for the integer template types as it is what I would expect to happen.

I tried to explain my reasoning in the inline comments. I don't feel strongly about that, but I do fell that makes a better API (it also mirrors what the underlying clang class does).

  • we should add default implementations for stuff in TypeSystem and not require all languages to override everything. I know this isn't the way things were done in the past, but we should fix that to avoid bloat in all TypeSystem subclasses.

Good idea. Will do.

include/lldb/Symbol/TypeSystem.h
356–360 ↗(On Diff #122247)

The second element of the pair is the *type* of the integral parameter (so you can e.g. differentiate between foo<47> and foo<47ULL>). I don't really have a use for it now, but it made sense to me to return this way (instead of through the "get type by index" function).

source/API/SBType.cpp
422 ↗(On Diff #122247)

Well.. I am not sure what would the SBType users expect, but I looked at all the in-tree CompilerType users. All of them are data formatters and they use it to pluck the element type out of e.g. std::vector<int>.

If somebody declares a type std::vector<47> for whatever reason, we wouldn't be doing them a favor by returning int to their question "what is the first template argument?". Some of the formatters (e.g. LibCxxInitializerList) actually check that the returned kind is Type, but most of them just take the result as face value. By returning the type only if it was a type template, we are making the formatters more correct and simpler.

That was my reasoning for CompilerType. I am not really sure how SBType is used out there, but my expectation is that it will be used in a similar way as we use CompilerType. I can see the logic in the other approach as well, but right now I can't think of any situation where I'd want to treat foo<int> and foo<47> in the same way.

zturner added inline comments.
unittests/Symbol/TestClangASTContext.cpp
418–419 ↗(On Diff #122247)

What about booleans, functions, pointers, references, parameter packs, and everything else? Even if you want to limit the scope of your change to exclude parameter packs, I think we should handle the other stuff. If it already works, can you add some tests for those cases?

I see your point. But if we do ask a template parameter for its type, I would like to be able to get it's type somehow. Seems wrong to leave this out. I know it doesn't mirror clang, but we should do the right thing here. At least at the SB API layer. I am fine with leaving the new TypeSystem calls as you added them.

labath added inline comments.Nov 9 2017, 11:13 AM
unittests/Symbol/TestClangASTContext.cpp
418–419 ↗(On Diff #122247)

I think boolean is handled as an integral type, but I can check that.

The other options here are "declaration", "template", "template expansion", "expression", "nullptr", and "pack".

Of these, I think I know what a "declaration" kind is, but I am not sure about most of the others (e.g. I don't think "expression" kind can even appear in our case, as we should always see a fully resolved template.

The thing is, there is nothing to "work" here. We have no API to retrieve and represent these entities. All you can do is call GetTemplateArgumentKind on them (which only translates between one enum and the other) and one of these two getters (which will return the empty value, because they're called on the wrong kind). I suppose I could test that.

What would be interesting is to test the other cases in the GetAsTemplateSpecialization (seeing through typedefs and stuff). This is actually the main reason why I wrote this test, as I had to refactor that code. I didn't do that, because I felt I would have to go through heroic efforts to construct e.g. a clang::Type::Elaborated object -- I don't think our ClangASTContext even has methods for constructing that.

I see your point. But if we do ask a template parameter for its type, I would like to be able to get it's type somehow. Seems wrong to leave this out. I know it doesn't mirror clang, but we should do the right thing here. At least at the SB API layer. I am fine with leaving the new TypeSystem calls as you added them.

If the thing your worried about is not being able to access the type of the integral template argument, I can add a new method to SBType which would return it's value and the type, mirroring the way TypeSystem does it (it would probably have to return something through an argument, as swig would probably choke on std::pair).

labath updated this revision to Diff 122407.Nov 10 2017, 2:40 AM
labath edited edge metadata.
labath marked 3 inline comments as done.
  • provide default no-op implementations in TypeSystem, so derived classes don't have to override them if they don't want to
  • have SBType::GetTemplateArgumentType return the type of the integral value for integral template arguments
  • more tests: I have added a test to check we correctly see through the "auto" type and that the functions (correctly) return empty values when called on an argument of the wrong kind.
clayborg accepted this revision.Nov 10 2017, 11:05 AM
This revision is now accepted and ready to land.Nov 10 2017, 11:05 AM
This revision was automatically updated to reflect the committed changes.