[ASTImporterTest] Make testing under '-fdelayed-template-parsing' mandatory

[a.sidorin]

Summary

While running ASTImporterTests, we often forget about Windows MSVC buildbots which enable '-fdelayed-template-parsing' by default. It takes reviewing time to find such issues as well as unexpected buildbot failures. To solve this issue, I suggest making '-fdelayed-template-parsing' mandatory so this problem can be caught during development.

[xazax.hun]

Is it possible that this will hide other problems? Wouldn't it be better to run the tests twice once with this argument and once without it?

[a.sidorin]

In D41444#960841, @xazax.hun wrote:

Is it possible that this will hide other problems? Wouldn't it be better to run the tests twice once with this argument and once without it?

I don't think so. In fact, without instantiation, we are not even able to check semantic code correctness inside templates. So, we are solving this problem as well.

[xazax.hun]

In D41444#960848, @a.sidorin wrote:

In D41444#960841, @xazax.hun wrote:

Is it possible that this will hide other problems? Wouldn't it be better to run the tests twice once with this argument and once without it?

I don't think so. In fact, without instantiation, we are not even able to check semantic code correctness inside templates. So, we are solving this problem as well.

E.g. the following code only compiles with -fdelayed-template-parsing flag added:

template<typename T>
struct Base {
  int x;
};


template<typename T>
struct Derived : Base<T> {
  int f() {
    return x;
  }
};

But yeah, maybe it is not very likely that we hit such issues.

[a.sidorin]

In case of -fdelayed-template-parsing, this code won't be even visible to the Importer. In my opinion, we shouldn't care about code we're not going to import. If we want to import it, we should make it visible and instantiate it. In this case it will not compile.
However, I completely agree with the statement that testing of two options is better. The question is how to design unit testing for different command line options. I'll make a try and update the patch. Unfortunately, the new version is much bigger than the source patch. I'm not also sure that new design is scalable if we want to introduce more options in future. Any suggestions on this are welcome.
Also, I still think we should rather not apply template-related patches until this testing is implemented. Gabor, Peter, do you agree?

[xazax.hun]

In D41444#960999, @a.sidorin wrote:

Also, I still think we should rather not apply template-related patches until this testing is implemented. Gabor, Peter, do you agree?

Sure, I am fine with that.

[zturner]

Can you just have getLangArgs return a vector of vectors, and then in testImport run it in a loop over all sets of args?

[a.sidorin]

Test both with and without '-fdelayed-template-parsing' in C++ mode.

[xazax.hun]

LGTM!

unittests/AST/ASTImporterTest.cpp

31

I wonder if in the future it would be worth to use something else, like LangOptions of clang instead of this enum. But I think this should be ok for now.

[a.sidorin]

unittests/AST/ASTImporterTest.cpp

31

This was inspired by ASTMatchersTest. Reusing existing enum is nice but it is an item for a separate patch, I think.

[szepet]

In D41444#961110, @a.sidorin wrote:

Test both with and without '-fdelayed-template-parsing' in C++ mode.

This solution LGTM as well.
Just a small nit added inline.

unittests/AST/ASTImporterTest.cpp

58

Nit: Maybe the order of the parameters could stay similar as the original: FromCode, FromArgs, ToCode, ToArgs, Verifier, AMatcher. (Or just Codes before Args)

It seems more intuitive to me.

unittests/AST/ASTImporterTest.cpp

 namespace ast_matchers {
 
 typedef std::vector<std::string> StringVector;
 
 void getLangArgs(Language Lang, StringVector &Args) {
   switch (Lang) {
   case Lang_C:
     Args.insert(Args.end(), { "-x", "c", "-std=c99" });
     break;
   case Lang_C89:
     Args.insert(Args.end(), { "-x", "c", "-std=c89" });
     break;
   case Lang_CXX:
-    Args.push_back("-std=c++98");
+    Args.insert(Args.end(), {"-std=c++98", "-fdelayed-template-parsing"});
     break;
   case Lang_CXX11:
-    Args.push_back("-std=c++11");
+    Args.insert(Args.end(), {"-std=c++11", "-fdelayed-template-parsing"});
     break;
   case Lang_OpenCL:
   case Lang_OBJCXX:
     break;
   }
 }
 
 template<typename NodeType, typename MatcherType>
 testing::AssertionResult
 testImport(const std::string &FromCode, Language FromLang,
            const std::string &ToCode, Language ToLang,
            MatchVerifier<NodeType> &Verifier,
            const MatcherType &AMatcher) {
   StringVector FromArgs, ToArgs;
   getLangArgs(FromLang, FromArgs);
   getLangArgs(ToLang, ToArgs);
 
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
   std::unique_ptr<ASTUnit>
       FromAST = tooling::buildASTFromCodeWithArgs(
         FromCode, FromArgs, InputFileName),
       ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
 
   ASTContext &FromCtx = FromAST->getASTContext(),
       &ToCtx = ToAST->getASTContext();