aaron.ballman added a comment. In D129748#3658657 <https://reviews.llvm.org/D129748#3658657>, @ChuanqiXu wrote:
> In D129748#3654918 <https://reviews.llvm.org/D129748#3654918>, @aaron.ballman > wrote: > >> For example, I would be IBOutletCollection, OwnerAttr, PointerAttr, and >> TypeTagForDatatypeAttr all behave the same way as they all take a type >> argument. I don't think we want to selectively disable so many attributes >> (for example, disallowing owner and pointer attributes means we lose out on >> C++ Core Guideline features that people will likely expect to be able to use >> with modules. > > I agree that there may be other problematic cases. But all of > `IBOutletCollection`,`OwnerAttr`, `PointerAttr` and `TypeTagForDatatypeAttr` > wouldn't meet the same problem. And from what I can see, now `PreferredName` > is special indeed. Since `PreferredName` is the only attribute whose argument > type is `TypeArgument<"TypedefType">`. The reason is stated below. All of the attributes I pointed to take a `TypeArgument`, so I'm still not certain I understand why you think these wouldn't be similarly impacted... >> Can you try again to explain the root problem? Perhaps with references to >> relevant code? > > Yeah, following of is my explanation to the root problem. It might be a > little bit long. First, for the following code: > > C++ > template<class _CharT> > class basic_string_view; > > typedef basic_string_view<char> string_view; > > template<class _CharT> > class > __attribute__((__preferred_name__(string_view))) > basic_string_view { > public: > basic_string_view() > { > } > }; > > inline basic_string_view<char> foo() > { > return basic_string_view<char>(); > } > > The dumped AST would be: > > |-ClassTemplateDecl 0x7913ac8 <./string_view.h:1:1, line:2:7> col:7 in > A.<global> hidden basic_string_view > | |-TemplateTypeParmDecl 0x7913950 <line:1:10, col:16> col:16 in A.<global> > hidden class depth 0 index 0 _CharT > | |-CXXRecordDecl 0x7913a18 <line:2:1, col:7> col:7 in A.<global> hidden > class basic_string_view > | `-ClassTemplateSpecializationDecl 0x7913cd0 <line:1:1, line:2:7> line:9:1 > in A.<global> 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 0x78c77b0 'char' > | |-PreferredNameAttr 0x79143c8 <line:8:16, col:46> string_view > | |-CXXRecordDecl 0x7914698 <line:7:1, line:9:1> col:1 in A.<global> > implicit class basic_string_view > | |-AccessSpecDecl 0x7914748 <line:10:1, col:7> col:1 in A.<global> public > | |-CXXConstructorDecl 0x7935158 <line:11:5, line:13:5> line:11:5 in > A.<global> used basic_string_view 'void ()' > | | `-CompoundStmt 0x79143a8 <line:12:5, line:13:5> > | |-CXXConstructorDecl 0x7935300 <line:9:1> col:1 in A.<global> implicit > constexpr basic_string_view 'void (const string_view &)' inline default > trivial noexcept-unevaluated 0x7935300 > | | `-ParmVarDecl 0x7935420 <col:1> col:1 in A.<global> 'const > string_view &' > | |-CXXConstructorDecl 0x79354d0 <col:1> col:1 in A.<global> implicit > constexpr basic_string_view 'void (string_view &&)' inline default trivial > noexcept-unevaluated 0x79354d0 > | | `-ParmVarDecl 0x79355f0 <col:1> col:1 in A.<global> 'string_view &&' > | `-CXXDestructorDecl 0x79357d0 <col:1> col:1 in A.<global> implicit > referenced constexpr ~basic_string_view 'void () noexcept' inline default > trivial > |-TypedefDecl 0x7913e78 <line:4:1, col:33> col:33 in A.<global> hidden > referenced string_view 'basic_string_view<char>':'string_view' > | `-TemplateSpecializationType 0x7913df0 'basic_string_view<char>' sugar > basic_string_view > | |-TemplateArgument type 'char' > | | `-BuiltinType 0x78c77b0 'char' > | `-RecordType 0x7913dd0 'string_view' > | `-ClassTemplateSpecialization 0x7913cd0 'basic_string_view' > |-ClassTemplateDecl 0x7914040 prev 0x7913ac8 <line:6:1, line:14:1> line:9:1 > in A.<global> hidden basic_string_view > | |-TemplateTypeParmDecl 0x7913ed8 <line:6:10, col:16> col:16 in A.<global> > hidden class depth 0 index 0 _CharT > | |-CXXRecordDecl 0x7913fa8 prev 0x7913a18 <line:7:1, line:14:1> line:9:1 > in A.<global> hidden 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 > | | |-PreferredNameAttr 0x7914100 <line:8:16, col:46> string_view > | | |-CXXRecordDecl 0x7914168 <line:7:1, line:9:1> col:1 in A.<global> > hidden implicit referenced class basic_string_view > | | |-AccessSpecDecl 0x7914218 <line:10:1, col:7> col:1 in A.<global> public > | | `-CXXConstructorDecl 0x79142d0 <line:11:5, line:13:5> line:11:5 in > A.<global> hidden basic_string_view<_CharT> 'void ()' > | | `-CompoundStmt 0x79143a8 <line:12:5, line:13:5> > | `-ClassTemplateSpecialization 0x7913cd0 'basic_string_view' > > Then when ASTReader reads `ClassTemplateDecl` at `0x7913ac8`, the ASTReader > would read `ClassTemplateSpecializationDecl` at `0x7913cd0`. Then the > ASTReader would try to read the `PreferredNameAttr` at `0x79143c8`. This > happens at > https://github.com/llvm/llvm-project/blob/557a471ab353d2be067d12b8cc352706ab089fc5/clang/lib/Serialization/ASTReaderDecl.cpp#L594-L600. > An important note here is that the ASTReader reads the attributes in the > very earlier state so that the ASTReader haven't read the name! (The > ASTReader would read the name of Decl in `ASTReaderDecl::VisitNamedDecl`, > which would happen after `ASTReaderDecl::VisitDecl`.) > > When the ASTReader reads `PreferredNameAttr`, it would try to read the > TypedefType since it's the argument type of `PreferredNameAttr`. The reading > code is generated from `clang/include/clang/AST/TypeProperties.td`: The argument type is `TypeArgument`, not `TypdefType`. The guts of the reading code for the preferred name attribute *should* be: case attr::PreferredName: { bool isInherited = Record.readInt(); bool isImplicit = Record.readInt(); bool isPackExpansion = Record.readInt(); TypeSourceInfo * typedefType = Record.readTypeSourceInfo(); New = new (Context) PreferredNameAttr(Context, Info, typedefType); cast<InheritableAttr>(New)->setInherited(isInherited); New->setImplicit(isImplicit); New->setPackExpansion(isPackExpansion); break; } > Long story short, the key problem here is that ASTReader don't take care of > the dependent loop. It looks like we can't fix the problem by a simple patch. > And the problem itself is really small. Currently only `PreferredNameAttr` > would trigger the problem. So it is an option to me to ignore the attribute > in the specific case. 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`. 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
