[clang] [clang-tools-extra] [clang] Improve nested name specifier AST representation (PR #147835)

[Aaron Ballman via cfe-commits]

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