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.
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
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?
Can you just have getLangArgs return a vector of vectors, and then in testImport run it in a loop over all sets of args?
LGTM!
unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
32 | 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. |
unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
32 | This was inspired by ASTMatchersTest. Reusing existing enum is nice but it is an item for a separate patch, I think. |
This solution LGTM as well.
Just a small nit added inline.
unittests/AST/ASTImporterTest.cpp | ||
---|---|---|
68 | 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. |
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.