[PATCH] D158766: [AST] TypeSourceInfo for params of inherited constructor should be null

[Sam McCall via Phabricator via cfe-commits]

sammccall added a reviewer: aaron.ballman.
sammccall marked 4 inline comments as done.
sammccall added a subscriber: aaron.ballman.
sammccall added a comment.

+Aaron for guidance & in case I'm making things up, TypeLocs/TSI have always confused me...



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13895-13896
       Context.getTrivialTypeSourceInfo(BaseCtor->getType(), UsingLoc);
   FunctionProtoTypeLoc ProtoLoc =
       TInfo->getTypeLoc().IgnoreParens().castAs<FunctionProtoTypeLoc>();
 
----------------
ymandel wrote:
> I'm now suspicious of all TypeLocs in this code, and If I'm reading this code correctly, this typeloc is never actually used.
My understanding is TypeLoc is really just a Type* + handle into some storage, TypeSourceInfo is typically that storage.
So TInfo is storage to hold the location info for a function type, initialized to ~empty ("trivial").
Then ProtoLoc is an accessor that we use to mutate that storage (`ProtoLoc.setParam` below).
Meanwhile we pass TInfo in as the TypeSourceInfo for the function.

The net effect: if we ask for the function's typeloc, and then ask that loc for the param decls, we'll get them, just the same as if we ask the function for its param decls directly.
That said, see next comment.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13908
   CXXConstructorDecl *DerivedCtor = CXXConstructorDecl::Create(
       Context, Derived, UsingLoc, NameInfo, TInfo->getType(), TInfo,
       BaseCtor->getExplicitSpecifier(), getCurFPFeatures().isFPConstrained(),
----------------
ymandel wrote:
> Once you're at it, should this use actually be nullptr?
... maybe? For implicit copy-constructors, the TSI is nullptr (SemaDeclCXX.cpp:15425).
That suggests that being able to get at the params via the TypeLoc isn't actually a critical invariant.

I've done this as I agree TypeSourceInfo for implicit code is dubious, and I was able to construct a similar ASTMatcher misbehavior that this fixes.

@aaron.ballman does this seem right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158766