[PATCH] D58673: [ASTImporter] Fix redecl failures of ClassTemplateSpec

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.


================
Comment at: lib/AST/ASTImporter.cpp:5147
       }
+    } else { // ODR violation.
+      // FIXME HandleNameConflict
----------------
shafik wrote:
> ODR violations are ill-formed no diagnostic required. So currently will this fail for cases that clang proper would not?
> ODR violations are ill-formed no diagnostic required.

ASTStructuralEquivalenceContext already provides diagnostic in the ODR cases. E.g.:
```
  // Check for equivalent field names.
  IdentifierInfo *Name1 = Field1->getIdentifier();
  IdentifierInfo *Name2 = Field2->getIdentifier();
  if (!::IsStructurallyEquivalent(Name1, Name2)) {
    if (Context.Complain) {
      Context.Diag2(Owner2->getLocation(),
                    Context.ErrorOnTagTypeMismatch
                        ? diag::err_odr_tag_type_inconsistent
                        : diag::warn_odr_tag_type_inconsistent)
          << Context.ToCtx.getTypeDeclType(Owner2);
      Context.Diag2(Field2->getLocation(), diag::note_odr_field_name)
          << Field2->getDeclName();
      Context.Diag1(Field1->getLocation(), diag::note_odr_field_name)
          << Field1->getDeclName();
    }
    return false;
  }
```
We change this to be always a Warning instead of an Error in this patch: https://reviews.llvm.org/D58897

> So currently will this fail for cases that clang proper would not?

Well, I think the situation is more subtle than that.
There are cases when we can link two translation units and the linker provides a valid executable even if there is an ODR violation of a type. There is no type information used during linkage, except for functions' mangled names and visibility. That ODR violation is recognized though when we do the merge in the AST level (and I think it is useful to diagnose them).
So if "clang proper" includes the linker then the answer is yes. If not then we are talking about only one translation unit and the answer is no.



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58673/new/

https://reviews.llvm.org/D58673