[PATCH] D47450: [ASTImporter] Use InjectedClassNameType at import of templated record.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

Hello Balázs,

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



================
Comment at: lib/AST/ASTImporter.cpp:2132
+        if (!DCXX->isInjectedClassName()) {
+          // In a record describing a template the type should be a
+          // InjectedClassNameType (see Sema::CheckClassTemplate). Update the
----------------
Nit: "an InjectedClassNameType".


================
Comment at: test/ASTMerge/injected-class-name-decl-1/Inputs/inject1.cpp:16
+} // namespace google
+namespace a {
+template <typename> class g;
----------------
martong wrote:
> balazske wrote:
> > a.sidorin wrote:
> > > 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.
> > 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.
> I vote on deleting this test then. We already have another clear and simple test.
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.


Repository:
  rC Clang

https://reviews.llvm.org/D47450