Page MenuHomePhabricator

[clang][ASTImporter] Fix for importing functions with EST_Unevaluated prototype.
ClosedPublic

Authored by balazske on Oct 18 2021, 9:12 AM.

Details

Summary

Fix for importing functions where the TypeSourceInfo is set and the
exception specification information contains reference to the function
declaration itself.

Diff Detail

Event Timeline

balazske created this revision.Oct 18 2021, 9:12 AM
balazske requested review of this revision.Oct 18 2021, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2021, 9:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Good job.

clang/unittests/AST/ASTImporterTest.cpp
6180

Perhaps, put here a FIXME to replace this with a real-world scenario.

6189

Please assert that the number of constructors is correct.
I would rather recommend doing so in the from case as well.

shafik added inline comments.Oct 19 2021, 8:41 AM
clang/unittests/AST/ASTImporterTest.cpp
6180

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.

balazske added inline comments.Oct 19 2021, 9:01 AM
clang/unittests/AST/ASTImporterTest.cpp
6180

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.

shafik added inline comments.Oct 19 2021, 10:03 AM
clang/unittests/AST/ASTImporterTest.cpp
6180

Yeah, that is what I meant, a test that did not require the setTypeSourceInfo hand crafting.

martong accepted this revision.Oct 20 2021, 8:17 AM

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.

clang/unittests/AST/ASTImporterTest.cpp
6180

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 ?

This revision is now accepted and ready to land.Oct 20 2021, 8:17 AM
balazske updated this revision to Diff 381154.Oct 20 2021, 11:54 PM

Improved test, set of TypeSourceInfo is not needed any more.

balazske marked 4 inline comments as done.Oct 20 2021, 11:59 PM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
6189

Here the goal is only to get a copy constructor, not to test if all constructors are imported. To get the constructor count relatively much additional code is needed (there is no function for it).

steakhal accepted this revision.Oct 21 2021, 12:13 AM

I love it.

clang/unittests/AST/ASTImporterTest.cpp
6189

I see. Thanks.

martong accepted this revision.Oct 21 2021, 4:40 AM

Thanks for updating the test case!