ChuanqiXu added a comment.

In D129748#3660684 <https://reviews.llvm.org/D129748#3660684>, @aaron.ballman 
wrote:

> The argument type is `TypeArgument`, not `TypdefType`. The guts of the 
> reading code for the preferred name attribute *should* be:

Oh, My bad. I didn't mention it in the above response. In 
https://github.com/llvm/llvm-project/blob/469044cfd355d34573643a57b5d2a78a9c341327/clang/lib/Sema/SemaDeclAttr.cpp#L1427-L1456,
 we could find that the type of `PreferredName` must be a type def type to a 
record type.

> I'm still not quite seeing that this only impacts one attribute. I *think* 
> this would happen for the others listed if the attribute argument was 
> similarly a typedef as with your example using `preferred_name`.

After experimenting the all day, I can't say the similar problem won't 
definitely happen to other attributes if they using a typedef as the attribute 
argument.

---

Here is the story of my experiment. The readers could skip this if not 
interested. `IBOutletCollection` is an Objective-C attributes. And the type 
argument of `TypeTagForDatatype` is required to be integral type: 
https://releases.llvm.org/9.0.0/tools/clang/docs/AttributeReference.html#type-tag-for-datatype.
  So let's see about `gsl::Owner` and `gsl::Pointer`. They are symmetric so 
let's see `gsl::Owner`.

First, I tried to following example, which replace [[gsl::Owner]] with 
`preferred_name`:

  template<class _CharT>
  class basic_string_view;
  
  typedef basic_string_view<char> string_view;
  
  template<class _CharT>
  class
  [[gsl::Owner(string_view)]]
  basic_string_view {
  public:
      basic_string_view() 
      {
      }
  };
  
  inline basic_string_view<char> foo()
  {
    return basic_string_view<char>();
  }

Then it don't meet the ODR violation problem. The reason is in the following 
AST:

  |-ClassTemplateDecl 0x7914038 <string_view.h:1:1, line:2:7> col:7 
basic_string_view
  | |-TemplateTypeParmDecl 0x7913ec0 <line:1:10, col:16> col:16 class depth 0 
index 0 _CharT
  | |-CXXRecordDecl 0x7913f88 <line:2:1, col:7> col:7 class basic_string_view
  | | `-OwnerAttr 0x7914670 <line:9:8, col:25> string_view
  | `-ClassTemplateSpecializationDecl 0x7914240 <line:1:1, line:2:7> line:10:1 
class basic_string_view definition
  |   |-DefinitionData pass_in_registers empty standard_layout 
trivially_copyable has_user_declared_ctor can_const_default_init
  |   | |-DefaultConstructor exists non_trivial user_provided 
defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param 
implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial
  |   | |-CopyAssignment simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial constexpr
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c7e50 'char'
  |   |-OwnerAttr 0x7914b98 <line:9:8, col:25> string_view
  |   |-CXXRecordDecl 0x7914c68 <line:7:1, line:10:1> col:1 implicit class 
basic_string_view
  |   |-AccessSpecDecl 0x7914d18 <line:11:1, col:7> col:1 public
  |   |-CXXConstructorDecl 0x7933d20 <line:12:5, line:14:5> line:12:5 used 
basic_string_view 'void ()'
  |   | `-CompoundStmt 0x7914978 <line:13:5, line:14:5>
  |   |-CXXConstructorDecl 0x7933ed0 <line:10:1> col:1 implicit constexpr 
basic_string_view 'void (const basic_string_view<char> &)' inline default 
trivial noexcept-unevaluated 0x7933ed0
  |   | `-ParmVarDecl 0x7933ff0 <col:1> col:1 'const basic_string_view<char> &'
  |   |-CXXConstructorDecl 0x79340a0 <col:1> col:1 implicit constexpr 
basic_string_view 'void (basic_string_view<char> &&)' inline default trivial 
noexcept-unevaluated 0x79340a0
  |   | `-ParmVarDecl 0x79341c0 <col:1> col:1 'basic_string_view<char> &&'
  |   `-CXXDestructorDecl 0x79343a0 <col:1> col:1 implicit referenced constexpr 
~basic_string_view 'void () noexcept' inline default trivial
  |-TypedefDecl 0x79143e8 <line:4:1, col:33> col:33 referenced string_view 
'basic_string_view<char>':'basic_string_view<char>'
  | `-TemplateSpecializationType 0x7914360 'basic_string_view<char>' sugar 
basic_string_view
  |   |-TemplateArgument type 'char'
  |   | `-BuiltinType 0x78c7e50 'char'
  |   `-RecordType 0x7914340 'basic_string_view<char>'
  |     `-ClassTemplateSpecialization 0x7914240 'basic_string_view'
  |-ClassTemplateDecl 0x79145b0 prev 0x7914038 <line:6:1, line:15:1> line:10:1 
basic_string_view
  | |-TemplateTypeParmDecl 0x7914448 <line:6:10, col:16> col:16 class depth 0 
index 0 _CharT
  | |-CXXRecordDecl 0x7914518 prev 0x7913f88 <line:7:1, line:15:1> line:10:1 
class basic_string_view definition
  | | |-DefinitionData empty standard_layout trivially_copyable 
has_user_declared_ctor can_const_default_init
  | | | |-DefaultConstructor exists non_trivial user_provided 
defaulted_is_constexpr
  | | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveConstructor exists simple trivial needs_implicit
  | | | |-CopyAssignment simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | | |-MoveAssignment exists simple trivial needs_implicit
  | | | `-Destructor simple irrelevant trivial constexpr needs_implicit
  | | |-OwnerAttr 0x79146d0 <line:9:8, col:25> string_view
  | | |-CXXRecordDecl 0x7914738 <line:7:1, line:10:1> col:1 implicit referenced 
class basic_string_view
  | | |-AccessSpecDecl 0x79147e8 <line:11:1, col:7> col:1 public
  | | `-CXXConstructorDecl 0x79148a0 <line:12:5, line:14:5> line:12:5 
basic_string_view<_CharT> 'void ()'
  | |   `-CompoundStmt 0x7914978 <line:13:5, line:14:5>
  | `-ClassTemplateSpecialization 0x7914240 'basic_string_view'

The most significant difference with `preferred_name` is: there is another 
`OwnerAttr` in `CXXRecordDecl` at `0x7913f88`. So it would read the attributes 
before it starts to read `ClassTemplateSpecializationDecl`. This is done by 
https://github.com/llvm/llvm-project/blob/469044cfd355d34573643a57b5d2a78a9c341327/clang/lib/Sema/SemaDeclAttr.cpp#L5051-L5052.
 When we handle `OwnerAttr`, it would add the attribute for every 
redeclaration. But `PreferredNameAttr` would only add the attribute for the 
declaration itself. So here is the problem.

Then I tried to add `PreferredNameAttr` for every redeclaration. But I met two 
problems.

(1) It would meet an infinite instantiation problem for the following case:

  C++
  template<int A, int B, typename ...T> struct Foo;
  template<typename ...T> using Bar = Foo<1, 2, T...>;
  using Bar2 = Foo<1, 2, int, float>;
  template<int A, int B, typename ...T> struct 
[[clang::preferred_name(::Bar<T...>)]] Foo {};
  Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' 
in 'preferred_name::Bar<int, float>'}}
  
  ::Foo<1, 2, int, float>::nosuch x; // expected-error {{no type named 'nosuch' 
in 'preferred_name::Bar<int, float>'}}

But `gsl::Owner` wouldn't meet the problem. And I haven't found the reason.

(2) It would crash when compiling stdmodules. The crash stack is relatively 
long and I haven't located the reason. So I omit them here.

---

@tahonermann @aaron.ballman As a summary, I admit this revision is something 
like "I met a problem that I couldn't solve so I tried to skip it instead of 
solving it". I understand this is generally bad. I have fought with the problem 
for about 2 weeks. Then I found things goes pretty good after I tried to skip 
it. Now the stdmodules have these examples: 
https://github.com/ChuanqiXu9/stdmodules/tree/master/examples. I know this is 
completely incomplete. But it should be a good direction. And I am trying to 
use it in actual projects, too. And the blocking issue is the `preferred_name`, 
which is a printer hinter to me. So I just feel like it may be not too bad to 
skip it. And clang15 is going to be released recently. It would be good to have 
something that people could have fun with. I know the current status of C++20 
Modules is not mature really. But it would be really helpful if some users are 
willing to eat dog foods to give feedbacks. It looks like I am the only one 
tester for C++20 Modules now.

It looks like @erichkeane likes the idea. How do you think about it 
@tahonermann  @aaron.ballman?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129748

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

Reply via email to