Page MenuHomePhabricator

[ASTImporter] Add comprehensive tests for ODR violation handling strategies
ClosedPublic

Authored by martong on Aug 29 2019, 8:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Aug 29 2019, 8:46 AM
martong updated this revision to Diff 217899.Aug 29 2019, 8:50 AM
  • Fix copy error
Harbormaster completed remote builds in B37518: Diff 217899.
balazske added inline comments.Sep 3 2019, 3:44 AM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
1 ↗(On Diff #217899)

Change the file name here?

martong updated this revision to Diff 218625.Sep 4 2019, 2:42 AM
  • Fix wrong file comment
martong marked an inline comment as done.Sep 4 2019, 2:43 AM
balazske added inline comments.Sep 6 2019, 6:13 AM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
36 ↗(On Diff #218625)

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?)

128 ↗(On Diff #218625)

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()));
  }
};
151 ↗(On Diff #218625)

FunctionTemplate and FunctionTemplateSpec are missing?

martong marked 6 inline comments as done.Sep 6 2019, 6:41 AM
martong added inline comments.
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
36 ↗(On Diff #218625)

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.

128 ↗(On Diff #218625)

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.

151 ↗(On Diff #218625)

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 ClassTemplateSpecializations 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?

OK
Probably the ClassTemplateSpec can not be handled in liberal way because the AST data structure for template specializations do not allow multiple instances with same argument values?

martong marked 3 inline comments as done.Sep 6 2019, 10:05 AM

OK
Probably the ClassTemplateSpec can not be handled in liberal way because the AST data structure for template specializations do not allow multiple instances with same argument values?

Yes exactly, that is the case. The implementation holds the specializations in a set, and the key within the set is a hash calculated from the arguments of the specialization.
All specializations are implemented like this, this includes variabletemplatespecializations too.

shafik added inline comments.Sep 10 2019, 9:51 PM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
118 ↗(On Diff #218625)

Note this is not well-formed b/c it is not initialized, see godbolt

shafik added inline comments.Sep 10 2019, 10:13 PM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
118 ↗(On Diff #218625)

But it would be ok combined w/ a specialization:

template <>
constexpr int X<int> = 0;
shafik added inline comments.Sep 10 2019, 10:29 PM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
151 ↗(On Diff #218625)

What you say about FunctionTemplateSpecializations and ClassTemplateSpecializations seems to make sense, importing them liberally would require more than work in the importer.

martong marked 3 inline comments as done.Sep 13 2019, 7:53 AM
martong added inline comments.
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
118 ↗(On Diff #218625)

Ok, I changed the template to have an init expression:

template <class T>
constexpr T X = 0;
151 ↗(On Diff #218625)

I have added FunctionTemplate, FunctionTemplateSpec and VarTemplateSpec.

FunctionTemplate decls overload with each other. Thus, they are imported always "liberally". I have added a test for that.

Class and variable template specializations/instantiatons are always imported conservatively, because the AST holds the specializations in a set, and the key within the set is a hash calculated from the arguments of the specialization.
I have added tests for class template specializations, but put the VarTemplateSpec tests into a FIXME, because structural eq does not handle VarTemplates and their specs yet.

Function template specializations are different. They are all "full"
specializations. Structural equivalency does not check the body of
functions, so we cannot create conflicting function template specializations.
Thus, ODR handling strategies has nothing to do with function template
specializations. I have added this as a comment to the tests.

martong updated this revision to Diff 220104.Sep 13 2019, 7:53 AM
martong marked an inline comment as done.
  • Initialize the variable template in test
  • Clean up tests for fun templates and for specializations
martong updated this revision to Diff 220130.Sep 13 2019, 10:01 AM
  • Reorganize ODRStrategy independent behaviour tests.

I have reorganized the tests to group the ones which test ODR violation strategy independent behaviour.

balazske added inline comments.Sep 16 2019, 12:50 AM
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
274 ↗(On Diff #220130)

This should be called ImportedF instead of PrevF?

martong updated this revision to Diff 220338.Sep 16 2019, 7:27 AM
martong marked 2 inline comments as done.
  • PrevF->ImportedF, remove braces
clang/unittests/AST/ASTImporterODRStrategiesTest.cpp
274 ↗(On Diff #220130)

Ok, renamed to ImportedF.
Also removed the superfluous braces.

balazske accepted this revision.Sep 17 2019, 7:10 AM
This revision is now accepted and ready to land.Sep 17 2019, 7:10 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 2:30 AM

Trying to fix in svn commit 372633.

2nd attempt to fix the windows build errors: 372646