[PATCH] D66951: [ASTImporter] Add comprehensive tests for ODR violation handling strategies

[Gabor Marton via Phabricator via cfe-commits]

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


================
Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:36
+  static constexpr auto *Definition = "void X(int a) {}";
+  static constexpr auto *ConflictingDefinition = "void X(double a) {}";
+  BindableMatcher<Decl> getPattern() {
----------------
balazske wrote:
> Probably for (non-member?) functions there is no possibility for conflicting definition (for same prototype)? This can be the case if the function body is different but this is not checked now. For functions no name conflict happens because of overload, at least if C++ is used. I think the tests here with functions are not needed or only for C language. (Do all these pass?)
We do check these only with lang C. That is handled with the `getLang()` function, each test use that language which is provided by the `getLang()` function.

`ConflictingDefinition` is needed, because we test cases like this (in `ImportConflictingDefAfterProto`):
```
void X(int); // TU1
void X(double a) {}; // TU2
```
This is handled differently by the distinct strategies. I.e. the conflicting definition is not imported when the strategy is `Conservative`. When it is `Liberal` then it is imported.


================
Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:128
+};
+
+struct ClassTemplateSpec {
----------------
balazske wrote:
> Is this better?
> ```
> struct VariableTemplate {
>   using DeclTy = VarTemplateDecl;
>   static constexpr auto *Prototype = "template <class T> extern T X;";
>   static constexpr auto *ConflictingPrototype =
>       "template <class T> extern float X;";
>   static constexpr auto *Definition =
>       R"(
>       template <class T> T X;
>       template <> int X<int>;
>       )";
>   static constexpr auto *ConflictingDefinition =
>       R"(
>       template <class T> T X;
>       template <> float X<int>;
>       )";
>   static constexpr auto *ConflictingProtoDef =
>       R"(
>       template <class T> float X;
>       template <> float X<int>;
>       )";
>   // There is no matcher for varTemplateDecl so use a work-around.
>   BindableMatcher<Decl> getPattern() {
>     return namedDecl(hasName("X"), unless(isImplicit()),
>                      has(templateTypeParmDecl()));
>   }
> };
> ```
I did not want to get this from our fork, becuase `template <> int X<int>;` seems to be a specialization of a variable template, and that confuses me.

Also, structural equivalency is completely wrong with variable templates, so they are here only for the sake of completeness, the tests which use them are all disabled.


================
Comment at: clang/unittests/AST/ASTImporterODRStrategiesTest.cpp:151
+};
+
+template <typename TypeParam, ASTImporter::ODRHandlingType ODRHandlingParam>
----------------
balazske wrote:
> `FunctionTemplate` and `FunctionTemplateSpec` are missing?
Yes, because `FunctionTemplates` overload with each other. So they are imported always "liberally".

There is no point to liberally import conflicting `FunctionTemplateSpecializations`.
The only thing we can do in that case is to omit the conflicting declaration.
And this is true in case of `ClassTemplateSpecialization`s too.

Perhaps we should remove `struct ClassTemplateSpec` as well from here (?).
Because they are never going to be handled "liberally".

@shafik , what do you think about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66951