AaronBallman wrote:
> > The comments in this area are confusing me, FWIW:
> > ```
> > /// Stores the TagDecl associated with this type. The decl may point to
> > any
> > /// TagDecl that declares the entity.
> > TagDecl *decl;
> >
> > ...
> >
> > TagDecl *getOriginalDecl() const { return decl; }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > The function is called "get original decl" which implies you get the first
> > declaration seen in the TU, but the data member it returns has a comment
> > saying it may point to any declaration in the redeclaration chain. One of
> > these is wrong, correct?
>
> Yeah, that is describing what is valid from the point of view of the AST Node.
>
> As a user of TagType, you can certainly create one which points to any
> declaration of an entity, and all of these nodes which point to a declaration
> of the same entity are the same type.
>
> From the point of view of Sema, there are further rules on how these types
> are created, in normal day-to-day source code parsing, the declaration
> pointed to by a non-canonical TagType will be the one found by lookup at that
> point in the program.
>
> FWIW the name `getOriginalDecl` was picked to temporarily disambiguate from
> the behavior of the original `getDecl` that existed before the patch.
>
> The difference in behavior is such that `getDecl` would always return the
> definition if that existed, otherwise it would return the very first
> declaration ever found by typename lookup when parsing a program, as there
> only existed one TagType per entity.
>
> The problem with keeping the name is that the behavior change meant that
> whenever I would rebase the patch, new users of getDecl would have popped up
> and it would be hard to make sure all uses of it were correct. By changing
> the name, I get a compilation error which would allow me to inspect and make
> the necessary changes.
Yeah, I think the plan is a reasonable one.
> As I stated before, once this patch is settled and everyone has had a nice
> window to rebase their upstream, my plan is to submit another patch renaming
> getOriginalDecl back to getDecl.
Okay, if this is just a temporary oddity, that's fine. My big concern is that
"original" implies "first" and that doesn't match the comment on what's
returned. Because temporary measures have a tendency to ossify sometimes, it
might make sense to add some more comments to clarify the situation. WDYT?
https://github.com/llvm/llvm-project/pull/147835
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
