This revision was automatically updated to reflect the committed changes.
Closed by commit rL321285: [ASTImporterTest] Add mandatory testing with
-fdelayed-template-parsing (authored by a.sidorin, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D41444?vs=127748=127903#toc
szepet accepted this revision.
szepet added a comment.
In https://reviews.llvm.org/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.
Comment at:
a.sidorin added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:32
+
+static RunOptions getRunOptionsForLanguage(Language Lang) {
+ ArgVector BasicArgs;
xazax.hun wrote:
> I wonder if in the future it would be worth to use something else, like
>
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: unittests/AST/ASTImporterTest.cpp:32
+
+static RunOptions getRunOptionsForLanguage(Language Lang) {
+ ArgVector BasicArgs;
I wonder
a.sidorin updated this revision to Diff 127748.
a.sidorin added a comment.
Test both with and without '-fdelayed-template-parsing' in C++ mode.
Repository:
rC Clang
https://reviews.llvm.org/D41444
Files:
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
zturner added a comment.
Can you just have `getLangArgs` return a vector of vectors, and then in
`testImport` run it in a loop over all sets of args?
Repository:
rC Clang
https://reviews.llvm.org/D41444
___
cfe-commits mailing list
xazax.hun added a comment.
In https://reviews.llvm.org/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.
Repository:
rC Clang
a.sidorin added a comment.
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.
On the other hand, it doesn’t hurt anything to run the tests twice does it?
I’d feel better if this patch were *adding* test coverage as opposed to
changing coverage
On Wed, Dec 20, 2017 at 6:50 AM Gábor Horváth via Phabricator <
revi...@reviews.llvm.org> wrote:
> xazax.hun added a comment.
>
>
xazax.hun added a comment.
In https://reviews.llvm.org/D41444#960848, @a.sidorin wrote:
> In https://reviews.llvm.org/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
a.sidorin added a comment.
In https://reviews.llvm.org/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
xazax.hun added a comment.
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?
Repository:
rC Clang
https://reviews.llvm.org/D41444
___
cfe-commits mailing
a.sidorin created this revision.
a.sidorin added reviewers: NoQ, xazax.hun, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
While running ASTImporterTests, we often forget about Windows MSVC buildbots
which enable '-fdelayed-template-parsing' by default. It takes reviewing time
to find
13 matches
Mail list logo
