Page MenuHomePhabricator

[ASTImporter] Use InjectedClassNameType at import of templated record.
ClosedPublic

Authored by balazske on May 28 2018, 5:20 AM.

Diff Detail

Repository
rC Clang

Event Timeline

balazske created this revision.May 28 2018, 5:20 AM
martong accepted this revision.May 28 2018, 5:28 AM

LGTM! But let's wait for someone else's review too.

@a.sidorin
We discovered this error during the CTU analysis of google/protobuf and we could reduce the erroneous C file with c-reduce to the minimal example presented in the test file.

This revision is now accepted and ready to land.May 28 2018, 5:28 AM

Adding Rafael too as a reviewer, because he has been working also on the ASTImporter recently.

r.stahl accepted this revision.May 28 2018, 6:30 AM

LGTM!

I'm not really too confident to approve changes yet, but with a second opinion it should be fine.

lib/AST/ASTImporter.cpp
2146

The only thing I'm wondering is whether the Decls looked up here are always known to be already imported.

balazske added inline comments.May 28 2018, 6:58 AM
lib/AST/ASTImporter.cpp
2146

These are in the 'To' context. It may be that the Injected is not found here, probably because not yet imported (in this case the import may be part of a not completed recursive process).

r.stahl added inline comments.May 28 2018, 7:22 AM
lib/AST/ASTImporter.cpp
2146

As far as I understand that corner case could be covered by doing the lookup on DCXX instead and then importing the injected decl. But then you wouldn't find it if it is only in the To context (if that is possible).

I mean if a user calls ImportDecl in another order specifically. But such a case is probably really artificial and I'm not sure if it's even makes sense or is already covered by ImportDeclParts.

It should be fine as it is.

a.sidorin requested changes to this revision.May 29 2018, 11:43 AM

Hello, Balázs,

You can find my comments inline.

lib/AST/ASTImporter.cpp
2138

Maybe we should fix it a bit upper (line 2100)?

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp
16 ↗(On Diff #148805)

This looks like raw creduce output. Is there a way to simplify this or make human-readable? Do we really need nested namespaces, unused decls and other stuff not removed by creduce? I know that creduce is bad at reducing templates because we often meet similar output internally. But it is usually not a problem to resolve some redundancy manually to assist creduce. In this case, we can start by removing k and n.
We can leave this code as-is only if removing declarations or simplifying templates affects import order causing the bug to disappear. But even in this case we have to humanize the test.

This revision now requires changes to proceed.May 29 2018, 11:43 AM
balazske requested review of this revision.May 30 2018, 6:53 AM
balazske added inline comments.
lib/AST/ASTImporter.cpp
2138

The ToDescribed is used in this code, not available before the new record is created.

balazske added inline comments.May 30 2018, 7:01 AM
test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp
16 ↗(On Diff #148805)

Probably remove this test? There was some bug in a previous version of the fix that was triggered by this test. Before that fix (and on current master) this test does not fail so it is not possible to simplify it.

martong added inline comments.May 30 2018, 7:45 AM
test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp
16 ↗(On Diff #148805)

I vote on deleting this test then. We already have another clear and simple test.

Hello Balázs,

Please clang-format the tests and delete injected-class-name-decl-1. Don't see any other issues.

lib/AST/ASTImporter.cpp
2139

Nit: "an InjectedClassNameType".

test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp
16 ↗(On Diff #148805)

I think we should delete this test. As I see, it passes even in upstream clang, so it doesn't really make sense to keep it.

balazske updated this revision to Diff 152634.Jun 25 2018, 1:35 AM

Small fixes.

balazske marked an inline comment as done.Jun 25 2018, 1:36 AM
This revision is now accepted and ready to land.Jun 26 2018, 3:14 AM
This revision was automatically updated to reflect the committed changes.