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
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
Can you just have getLangArgs return a vector of vectors, and then in testImport run it in a loop over all sets of args?
Comment Actions
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. |
| 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. |
Comment Actions
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. |