[cling] Unload implicit instantiations also from ASTFile #21043
Conversation
The assumption is that serialized declarations must be valid, however that doesn't hold anymore in the presence of static_assert in templated classes. Fixes root-project#20638
| if (D->isFromASTFile()) { | ||
| // We cannot ignore instantiated CXXMethodDecl's in templated classes: | ||
| // They may have failing static_assert. | ||
| if (auto* M = dyn_cast<CXXMethodDecl>(D)) { |
There was a problem hiding this comment.
Is that change sufficient for these things to be reinstantiated? In particular, non-static_assert method decls coming from ast files? We should probably add some tests.
There was a problem hiding this comment.
This could be relatively easy, right? If I understand correctly we need a template broghut into memory from a pcm and then 2 instantiations in a row - please correct me if I am wrong.
If the above is correct, and this cannot be put in cling since we need a dictionary, we can comfortably put the test in roottest, maybe even profiting from ROOT helpers that at runtime allow us to generate dictionaries on the fly
There was a problem hiding this comment.
Yes, Dev has a test case (in roottest) for the problem I'm trying to solve. We don't have a (new) test for an unloaded method that does not have a static_assert (I believe that's what Vassil is asking about). It's probably covered by other tests, but I need to check. Moreover there are two failing tests (roottest-cling-reload-reloadvector and roottest-root-tree-selector-selector) that I don't have locally...
There was a problem hiding this comment.
Yes, having tests in cling is better because when we break this it's far easier to debug. The test does not need a dictionary. We need a module file with a static assert declaration.
There was a problem hiding this comment.
We don't have a single test using modules / pcm's in interpreter/cling/test and I'm not going to invent the infrastructure for this PR. The test is going to go into roottest.
There was a problem hiding this comment.
Thank you for being constructive. In that case please hold off merging this PR until I provide you with the infrastructure (which is 3 lines).
There was a problem hiding this comment.
In fact the infrastructure seems to be there already. Please take a look at cling/test/CodeUnloading/PCH/. We should be able to extend some of these tests to cover that case, too.
Test Results 22 files 22 suites 3d 15h 39m 15s ⏱️ For more details on these failures, see this check. Results for commit f83ce07. |
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
This PR needs the appropriate test in cling. I will provide the infrastructure in the next week or so.
3 participants
The assumption is that serialized declarations must be valid, however that doesn't hold anymore in the presence of static_assert in templated classes.
Fixes #20638