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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13929
         Context, DerivedCtor, UsingLoc, UsingLoc, /*IdentifierInfo=*/nullptr,
-        FPT->getParamType(I), TInfo, SC_None, /*DefArg=*/nullptr);
+        FPT->getParamType(I), /*TInfo=*/nullptr, SC_None, /*DefArg=*/nullptr);
     PD->setScopeInfo(0, I);
----------------
I think this change is potentially incorrect, at least judging by: https://github.com/llvm/llvm-project/blob/ce0d16f574f5bd81086e124345b0fe8dc4bfd95d/clang/lib/Sema/SemaDeclCXX.cpp#L15442

But then again: https://github.com/llvm/llvm-project/blob/ce0d16f574f5bd81086e124345b0fe8dc4bfd95d/clang/lib/Sema/SemaDeclCXX.cpp#L15584

So perhaps more investigation is needed here?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13908
   CXXConstructorDecl *DerivedCtor = CXXConstructorDecl::Create(
       Context, Derived, UsingLoc, NameInfo, TInfo->getType(), TInfo,
       BaseCtor->getExplicitSpecifier(), getCurFPFeatures().isFPConstrained(),
----------------
sammccall wrote:
> 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?
Errr... somewhere in the middle between right and wrong. I can see why `TypeSourceInfo` for implicit code is dubious, but we've had a theoretical long-standing goal that we've never seriously worked towards of replacing `QualType`/`Type *` with `TypeSourceInfo *` in many of our interfaces so that location information about types is more readily available.

On balance, I think passing `nullptr` here is reasonable -- this is an implicit node and we do this for other implicitly created special member functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158766