Page MenuHomePhabricator

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

Authored by a.sidorin on Dec 20 2017, 6:29 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin created this revision.Dec 20 2017, 6:29 AM

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?

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.

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.

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?

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.

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 updated this revision to Diff 127748.Dec 20 2017, 9:38 AM

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

xazax.hun accepted this revision.Dec 21 2017, 4:44 AM

LGTM!

unittests/AST/ASTImporterTest.cpp
32 ↗(On Diff #127748)

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.

This revision is now accepted and ready to land.Dec 21 2017, 4:44 AM
a.sidorin added inline comments.Dec 21 2017, 5:09 AM
unittests/AST/ASTImporterTest.cpp
32 ↗(On Diff #127748)

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

szepet accepted this revision.Dec 21 2017, 6:44 AM

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
68 ↗(On Diff #127748)

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.

This revision was automatically updated to reflect the committed changes.