[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Michael Buch via Phabricator via cfe-commits]

Michael137 abandoned this revision.
Michael137 added a comment.

Abandoning in favour of https://reviews.llvm.org/D141826


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D140423#4055722 , @Michael137 
wrote:

> In D140423#4052442 , @aaron.ballman 
> wrote:
>
>> In D140423#4052271 , @dblaikie 
>> wrote:
>>
>>> In D140423#4051262 , 
>>> @aaron.ballman wrote:
>>>
> Add something like a bool IsDefaulted somewhere in Clang, e.g., in 
> TemplateArgument and consult it from the TypePrinter. This would be much 
> simpler but requires adding a field on one of the Clang types

 I think this might be worth exploring as a cleaner solution to the 
 problem. `TemplateArgument` has a union of structures for the various 
 kinds of template arguments it represents 
 (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140).
  All of the structures in that union start with an `unsigned Kind` field 
 to discriminate between the members. There are only 8 kinds currently 
 (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63),
  so we could turn `Kind` into a bit-field and then steal a bit for 
 `IsDefaulted` without increasing memory overhead. Do you think that's a 
 reasonable approach to try instead?
>>>
>>> FWIW, I Think I discouraged @Michael137 from going down that direction 
>>> since it felt weird to me to add that sort of thing to the Clang ASTs for 
>>> an lldb-only use case, and a callback seemed more suitable. But this is 
>>> hardly my wheelhouse - if you reckon that's the better direction, carry on, 
>>> I expect @Michael137 will be on board with that.
>>
>> Adding in @erichkeane as templates code owner in case he has opinions.
>>
>> I agree it's a bit weird to modify the AST only for lldb only, but adding a 
>> callback to the printing policy is basically an lldb-only change as well (I 
>> don't imagine folks would use that callback all that often). So my thinking 
>> is that if we can encode the information in the AST for effectively zero 
>> cost, that helps every consumer of the AST (thinking about things like 
>> clang-tidy) and not just people printing. However, this is not a strongly 
>> held position, so if there's a preference for the current approach, it seems 
>> workable to me.
>
> Thanks for taking a look @aaron.ballman @erichkeane
>
> I prepared an alternative draft patch series with your suggestion of adding 
> an `IsDefaulted` bit to `TemplateArgument`:
>
> - https://reviews.llvm.org/D141826
> - https://reviews.llvm.org/D141827
>
> Is this what you had in mind?
>
> This *significantly* simplifies the LLDB support: 
> https://reviews.llvm.org/D141828
>
> So I'd prefer this over the callback approach TBH.
>
>> A Class template instantiation SHOULD have its link back to the class 
>> template, and should be able to calculate whether the template argument is 
>> defaulted, right? At least if it is the SAME as the default (that is, I'm 
>> not sure how well we can tell the difference between a defaulted arg, and a 
>> arg set to the default value).
>>
>> I wouldn't expect a bool to be necessary, and I see 
>> isSubstitutedDefaultArgument seems to do that work, right?
>
> As @dblaikie mentioned, unfortunately LLDB currently isn't able to construct 
> the `ClassTemplateDecl` in a way where `isSubstitutedDefaultArgument` would 
> correctly deduce whether a template instantiation has defaulted arguments. In 
> DWARF we only have info about a template instantiation (i.e., the structure 
> of the generic template parameters is not encoded). So we can't supply 
> `(Non)TypeTemplateParamDecl::setDefaultArgument` with the generic arguments 
> Clang expects. The `ClassTemplateDecl` parameters are set up here: 
> https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L1391-L1402
>
> Regardless of how complex the template parameters get, LLDB just turns each 
> into a plain `(Non)TypeTemplateParamDecl`

Got it, I missed that part, I'm not too familiar with how LLDB works in this 
case.  I'll take a look at those other patches.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Michael Buch via Phabricator via cfe-commits]

Michael137 added a comment.

In D140423#4052442 , @aaron.ballman 
wrote:

> In D140423#4052271 , @dblaikie 
> wrote:
>
>> In D140423#4051262 , 
>> @aaron.ballman wrote:
>>
 Add something like a bool IsDefaulted somewhere in Clang, e.g., in 
 TemplateArgument and consult it from the TypePrinter. This would be much 
 simpler but requires adding a field on one of the Clang types
>>>
>>> I think this might be worth exploring as a cleaner solution to the problem. 
>>> `TemplateArgument` has a union of structures for the various kinds of 
>>> template arguments it represents 
>>> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140).
>>>  All of the structures in that union start with an `unsigned Kind` field to 
>>> discriminate between the members. There are only 8 kinds currently 
>>> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63),
>>>  so we could turn `Kind` into a bit-field and then steal a bit for 
>>> `IsDefaulted` without increasing memory overhead. Do you think that's a 
>>> reasonable approach to try instead?
>>
>> FWIW, I Think I discouraged @Michael137 from going down that direction since 
>> it felt weird to me to add that sort of thing to the Clang ASTs for an 
>> lldb-only use case, and a callback seemed more suitable. But this is hardly 
>> my wheelhouse - if you reckon that's the better direction, carry on, I 
>> expect @Michael137 will be on board with that.
>
> Adding in @erichkeane as templates code owner in case he has opinions.
>
> I agree it's a bit weird to modify the AST only for lldb only, but adding a 
> callback to the printing policy is basically an lldb-only change as well (I 
> don't imagine folks would use that callback all that often). So my thinking 
> is that if we can encode the information in the AST for effectively zero 
> cost, that helps every consumer of the AST (thinking about things like 
> clang-tidy) and not just people printing. However, this is not a strongly 
> held position, so if there's a preference for the current approach, it seems 
> workable to me.

Thanks for taking a look @aaron.ballman

I prepared an alternative draft patch series with your suggestion of adding an 
`IsDefaulted` bit to `TemplateArgument`:

- https://reviews.llvm.org/D141826
- https://reviews.llvm.org/D141827

Is this what you had in mind?

This *significantly* simplifies the LLDB support: 
https://reviews.llvm.org/D141828

So I'd prefer this over the callback approach TBH.

> A Class template instantiation SHOULD have its link back to the class 
> template, and should be able to calculate whether the template argument is 
> defaulted, right? At least if it is the SAME as the default (that is, I'm not 
> sure how well we can tell the difference between a defaulted arg, and a arg 
> set to the default value).
>
> I wouldn't expect a bool to be necessary, and I see 
> isSubstitutedDefaultArgument seems to do that work, right?

As @dblaikie mentioned, unfortunately LLDB currently isn't able to construct 
the `ClassTemplateDecl` in a way where this `isSubstitutedDefaultArgument` 
would correctly deduce whether a template instantiation has defaulted 
arguments. In DWARF we only have info about a template instantiation, but the 
structure of the generic template parameters is not encoded. So we can't supply 
`(Non)TypeTemplateParamDecl::setDefaultArgument` with the generic arguments 
Clang expects. The `ClassTemplateDecl` parameters are set up here: 
https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L1391-L1402

Regardless of how complex the template parameters get, LLDB just turns each 
into a plain `(Non)TypeTemplateParamDecl`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D140423#4052540 , @erichkeane 
wrote:

> A Class template instantiation SHOULD have its link back to the class 
> template, and should be able to calculate whether the template argument is 
> defaulted, right?

For a normal/clang-built AST, yes. But as mentioned in this review description, 
for LLDB, DWARF doesn't encode enough information to make the class template 
properly. So it only knows if a given class template specialization's argument 
is defaulted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

A Class template instantiation SHOULD have its link back to the class template, 
and should be able to calculate whether the template argument is defaulted, 
right?  At least if it is the SAME as the default (that is, I'm not sure how 
well we can tell the difference between a defaulted arg, and a arg set to the 
default value).

I wouldn't expect a bool to be necessary, and I see 
`isSubstitutedDefaultArgument` seems to do that work, right?

So I guess I wonder why 'print the defaulted template args' isn't just a 
printing-policy?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a subscriber: erichkeane.
aaron.ballman added a comment.

In D140423#4052271 , @dblaikie wrote:

> In D140423#4051262 , @aaron.ballman 
> wrote:
>
>>> Add something like a bool IsDefaulted somewhere in Clang, e.g., in 
>>> TemplateArgument and consult it from the TypePrinter. This would be much 
>>> simpler but requires adding a field on one of the Clang types
>>
>> I think this might be worth exploring as a cleaner solution to the problem. 
>> `TemplateArgument` has a union of structures for the various kinds of 
>> template arguments it represents 
>> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140).
>>  All of the structures in that union start with an `unsigned Kind` field to 
>> discriminate between the members. There are only 8 kinds currently 
>> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63),
>>  so we could turn `Kind` into a bit-field and then steal a bit for 
>> `IsDefaulted` without increasing memory overhead. Do you think that's a 
>> reasonable approach to try instead?
>
> FWIW, I Think I discouraged @Michael137 from going down that direction since 
> it felt weird to me to add that sort of thing to the Clang ASTs for an 
> lldb-only use case, and a callback seemed more suitable. But this is hardly 
> my wheelhouse - if you reckon that's the better direction, carry on, I expect 
> @Michael137 will be on board with that.

Adding in @erichkeane as templates code owner in case he has opinions.

I agree it's a bit weird to modify the AST only for lldb only, but adding a 
callback to the printing policy is basically an lldb-only change as well (I 
don't imagine folks would use that callback all that often). So my thinking is 
that if we can encode the information in the AST for effectively zero cost, 
that helps every consumer of the AST (thinking about things like clang-tidy) 
and not just people printing. However, this is not a strongly held position, so 
if there's a preference for the current approach, it seems workable to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D140423#4051262 , @aaron.ballman 
wrote:

>> Add something like a bool IsDefaulted somewhere in Clang, e.g., in 
>> TemplateArgument and consult it from the TypePrinter. This would be much 
>> simpler but requires adding a field on one of the Clang types
>
> I think this might be worth exploring as a cleaner solution to the problem. 
> `TemplateArgument` has a union of structures for the various kinds of 
> template arguments it represents 
> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140).
>  All of the structures in that union start with an `unsigned Kind` field to 
> discriminate between the members. There are only 8 kinds currently 
> (https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63),
>  so we could turn `Kind` into a bit-field and then steal a bit for 
> `IsDefaulted` without increasing memory overhead. Do you think that's a 
> reasonable approach to try instead?

FWIW, I Think I discouraged @Michael137 from going down that direction since it 
felt weird to me to add that sort of thing to the Clang ASTs for an lldb-only 
use case, and a callback seemed more suitable. But this is hardly my wheelhouse 
- if you reckon that's the better direction, carry on, I expect @Michael137 
will be on board with that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

> Add something like a bool IsDefaulted somewhere in Clang, e.g., in 
> TemplateArgument and consult it from the TypePrinter. This would be much 
> simpler but requires adding a field on one of the Clang types

I think this might be worth exploring as a cleaner solution to the problem. 
`TemplateArgument` has a union of structures for the various kinds of template 
arguments it represents 
(https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L140).
 All of the structures in that union start with an `unsigned Kind` field to 
discriminate between the members. There are only 8 kinds currently 
(https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/TemplateBase.h#L63),
 so we could turn `Kind` into a bit-field and then steal a bit for 
`IsDefaulted` without increasing memory overhead. Do you think that's a 
reasonable approach to try instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/include/clang/AST/PrettyPrinter.h:52
+
+  enum class TriState : int { kYes, kNo, kUnknown };
+

We don't usually use the `k` prefix for enums (the style guide mentions using 
an acronym like `TS_` - though even that's unnecessary with an enum class, 
where you have to use `EnumClassName` prefix anyway, so there's no issue with 
ambiguity/name collisions of the enumerators)

But also, I'm guessing we probably use `std::optional` for this sort of 
thing more frequently than defining a three-state enum (even though 
`std::optional` can be a bit awkward to use/error prone, it's not too bad 
for limited uses like this).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140423/new/

https://reviews.llvm.org/D140423

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140423: [WIP][clang] Add PrintingPolicy callback for identifying default template arguments

[Michael Buch via Phabricator via cfe-commits]

Michael137 created this revision.
Michael137 added reviewers: aprantl, dblaikie.
Herald added a project: All.
Michael137 added a comment.
Michael137 updated this revision to Diff 484341.
Michael137 added a reviewer: labath.
Michael137 edited the summary of this revision.
Michael137 added a reviewer: aaron.ballman.
Michael137 updated this revision to Diff 484434.
Michael137 updated this revision to Diff 484439.
Michael137 published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

An example of how this would be used from LLDB is: 
https://reviews.llvm.org/D140145


Michael137 added a comment.

- Remove debug print in test


aprantl added a comment.

Overall this seems to have a pretty small surface area, so I'd be fine with 
including this.


Michael137 added a comment.

@aaron.ballman we're trying to fix printing of C++ types with default template 
arguments in LLDB. Currently DWARF doesn't have the necessary info to construct 
a `ClassTemplateDecl` with generic parameters, which means the logic for 
suppressing them in the `TypePrinter` doesn't quite work.

This patch outlines of one approach we considered. We add a hook into the 
TypePrinter to allow external AST sources to answer the question "is template 
argument X defaulted?".

Another alternative would be to have a `TemplateArgument::IsDefaulted` which 
gets set in LLDB and read from the TypePrinter.

Do you think either of these approaches would be fine for inclusion?


Michael137 added a comment.

- Fix test. Move variable into loop


Michael137 added a comment.

- Rebase


Michael137 added a comment.

putting into review queue to hear people's opinion on something along these 
lines




Comment at: clang/include/clang/AST/PrettyPrinter.h:52
+
+  enum class TriState : int { kYes, kNo, kUnknown };
+

The TrisState is needed because LLDB has two AST sources:
1. DWARF
2. clang modules

When we import a type from a clang module we would never have consulted DWARF 
about whether a parameter was defaulted. So instead of more complex bookkeeping 
it seemed simpler to say "if we've never seen this decl when parsing DWARF, I 
can't say anything about the default-ness of the arguments"



Comment at: clang/lib/AST/TypePrinter.cpp:2095
+
+  while (!Args.empty()) {
+if (Callbacks != nullptr)

maybe this body becomes slightly less oddly indented when converting it to a 
range-based for with a counter?


This patch adds a hook into the `TypePrinter` to allow clients
decide whether a template argument corresponds to the default
parameter.

**Motivation**

DWARF encodes information about template instantiations, but
not about the generic definition of a template. If an argument
in a template instantiation corresponds to the default parameter
of the generic template then DWARF will attach a `DW_AT_default_value`
to that argument. LLDB uses the Clang TypePrinter for
displaying/formatting C++ types but currently can't give Clang the 
`ClassTemplateDecl`
that it expects. Since LLDB does know whether a particular instantiation has
default arguments, this patch allows LLDB (and other clients with external AST 
sources)
to help Clang in identifying those default arguments.

**Alternatives**

1. Encode information about generic template definitions in DWARF. It's unclear 
how long it would take to get this proposed and implemented and whether 
something like this would work in the first place. If we could construct 
`ClassTemplateDecl`s with the correct generic parameters this patch wouldn't be 
required.

2. Add something like a `bool IsDefaulted` somewhere in Clang, e.g., in 
`TemplateArgument` and consult it from the `TypePrinter`. This would be much 
simpler but requires adding a field on one of the Clang types


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140423

Files:
  clang/include/clang/AST/PrettyPrinter.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/TypePrinter.cpp
  clang/unittests/AST/TypePrinterTest.cpp

Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -127,3 +127,39 @@
 Policy.EntireContentsOfLargeArray = true;
   }));
 }
+
+TEST(TypePrinter, TemplateParameterListWithCallback) {
+  constexpr char Code[] = R"cpp(
+template 
+struct Foo {   
+}; 
+   
+Foo X;   
+  )cpp";
+  auto Matcher = classTemplateSpecializationDecl(
+  hasName("Foo"), has(cxxConstructorDecl(
+  has(parmVarDecl(hasType(qualType().bind("id")));
+
+  struct DefaulterCallback final : public PrintingCallbacks {
+virtual TriState
+IsTemplateArgumentDefaulted(clang::ClassTemplateSpecializationDecl const *,
+