martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Thanks! The fix makes sense and looks good to me. Though, it would be nice if
you could provide a test without the hand crafting of the setTypeSourceInfo,
but I think that could be done in a follow-up patch.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6180
+ ASSERT_FALSE(FromCtor->getTypeSourceInfo());
+ // Set a TypeSourceInfo for the function, this state may occur in reality.
+ TypeSourceInfo *FromTSI = FromTU->getASTContext().getTrivialTypeSourceInfo(
----------------
shafik wrote:
> balazske wrote:
> > shafik wrote:
> > > steakhal wrote:
> > > > Perhaps, put here a `FIXME` to replace this with a real-world scenario.
> > > I think it is useful to have cases that we run into in practice as a
> > > regression test but it would be nice to see a more general test as well.
> > What means a "more general test"? Code where the `setTypeSourceInfo` is not
> > needed? I just could not find out why there is no TypeSourceInfo set. The
> > involved code part looks like this (comes from Bitcoin):
> > ```
> > void
> > RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface>
> > pwalletIn) {
> > // Each connection captures pwalletIn to ensure that each callback is
> > // executed before pwalletIn is destroyed. For more details see #18338.
> > g_signals.m_internals->Register(std::move(pwalletIn));
> > }
> >
> > void RegisterValidationInterface(CValidationInterface* callbacks)
> > {
> > // Create a shared_ptr with a no-op deleter - CValidationInterface
> > lifecycle
> > // is managed by the caller.
> > RegisterSharedValidationInterface({callbacks,
> > [](CValidationInterface*){}});
> > // ^ This is the lambda to
> > be imported.
> > }
> > ```
> > If the same code is used in the test and a simple `CValidationInterface`
> > and `std::shared_ptr` with appropriate constructor is added there is still
> > no `TypeSourceInfo` in the needed place.
> Yeah, that is what I meant, a test that did not require the
> `setTypeSourceInfo` hand crafting.
On one hand, I think this is a through and precisely written test case and we
had similar test cases before. On the other hand, I agree it is unfortunate
that we have to change the AST in the test setup.
@balazske Perhaps creduce-ing the to be imported TU with the bitcoin case could
provide a reduced case that does not require the hand crafting of the
`setTypeSourceInfo` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112013/new/
https://reviews.llvm.org/D112013
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
