Page MenuHomePhabricator

[ASTImporter] Use InjectedClassNameType at import of templated record.

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

Diff Detail

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.

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


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


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

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

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.


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


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.

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

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

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.


Nit: "an InjectedClassNameType".


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.