This revision was automatically updated to reflect the committed changes.
Closed by commit rC328906: [ASTImporter] Add test helper Fixture (authored by
szepet, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D43967?vs=139791&id=140497#toc
Repository:
rC Clang
https://revie
martong added a comment.
Ping.
Could someone with commit rights please commit this? @szepet or @xazax.hun ?
Repository:
rC Clang
https://reviews.llvm.org/D43967
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bi
martong updated this revision to Diff 139791.
martong added a comment.
- Fix gcc 5 warnings
Repository:
rC Clang
https://reviews.llvm.org/D43967
Files:
unittests/AST/ASTImporterTest.cpp
unittests/AST/DeclMatcher.h
Index: unittests/AST/DeclMatcher.h
==
martong added a comment.
Hi Aleksei,
I don't have commit permissions, could you please help and commit?
Repository:
rC Clang
https://reviews.llvm.org/D43967
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/ma
martong added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:276
+// This will not create the file more than once.
+createVirtualFile(ToAST.get(), It->FileName, It->Code);
+
xazax.hun wrote:
> Maybe an IfNeeded suffix or something like tha
martong updated this revision to Diff 139567.
martong marked 3 inline comments as done.
martong added a comment.
- Add some minor changes based on xazax's comments
Repository:
rC Clang
https://reviews.llvm.org/D43967
Files:
unittests/AST/ASTImporterTest.cpp
unittests/AST/DeclMatcher.h
I
xazax.hun accepted this revision.
xazax.hun added a comment.
Overall looks good, some minor comments inline.
Comment at: unittests/AST/ASTImporterTest.cpp:276
+// This will not create the file more than once.
+createVirtualFile(ToAST.get(), It->FileName, It->Code);
+
--
a.sidorin accepted this revision.
a.sidorin added a comment.
This revision is now accepted and ready to land.
This looks good to me. Thank you!
@xazax.hun Gabor, could you please take a look?
Repository:
rC Clang
https://reviews.llvm.org/D43967
_
martong added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:1169
+ MatchVerifier Verifier;
+ ASSERT_TRUE(Verifier.match(From, cxxRecordDecl(has(fieldDecl();
+ ASSERT_TRUE(Verifier.match(To, cxxRecordDecl(has(fieldDecl();
a.sidorin wrot
martong updated this revision to Diff 139295.
martong marked 7 inline comments as done.
martong added a comment.
Add minor syntax changes and rename
Repository:
rC Clang
https://reviews.llvm.org/D43967
Files:
unittests/AST/ASTImporterTest.cpp
unittests/AST/DeclMatcher.h
Index: unittests
a.sidorin added a comment.
Hi Gabor! This looks much better.
Comment at: unittests/AST/ASTImporterTest.cpp:1000
+ R"(
+ template
+ struct X {};
In C++, names started with underscore are reserved for implementation so it's
undesirable to use them
martong added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:198
+ Language ToLang, StringRef Identifier = "declToImport") {
+ArgVector FromArgs = getBasicRunOptionsForLanguage(FromLang),
+ ToArgs = getBasicRunOptionsForLanguage(T
martong updated this revision to Diff 139181.
martong marked 14 inline comments as done.
martong added a comment.
Update based on Alexei's comments.
- Use createVirtualFile in testImport
- Add minimal style changes
- Use DeclCounter
- Add hasFieldOrder matcher
- Add parameterized tests
Reposito
a.sidorin added a comment.
Hello Gabor,
Sorry for the delay with review: the patch is pretty big and it was hard to
find time for reviewing it.
I like the patch but it requires some cleanup like formatting changes. Could
you please clang-format new code?
Also, I'd like to avoid code duplication
martong added inline comments.
Comment at: unittests/AST/ASTImporterTest.cpp:170
+ASTContext &ToCtx = ToAST->getASTContext();
+vfs::OverlayFileSystem *OFS = static_cast(
+
ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
xa
martong updated this revision to Diff 137563.
martong marked 15 inline comments as done.
martong added a comment.
Fix code review comments.
Repository:
rC Clang
https://reviews.llvm.org/D43967
Files:
unittests/AST/ASTImporterTest.cpp
unittests/AST/DeclMatcher.h
Index: unittests/AST/Decl
xazax.hun added a comment.
I think having both kinds of tests might make sense.
Overall, this looks good to me. Some nits inline.
Comment at: unittests/AST/ASTImporterTest.cpp:143
+class Fixture : public ::testing::Test {
+
I do not like the name of this clas
martong added a comment.
Hi Aleksei,
I agree that there are some overlapping functionalities in the patches.
As far as I see both make it possible to write tests which import from multiple
contexts and I think that is the only redundant part. So, I agree, for now we
could use both approaches si
a.sidorin added a comment.
Hello Gabor,
Unfortunately, we did same or similar work: https://reviews.llvm.org/D44079.
We have to decide what approach is going to be used.
It doesn't mean that we should select only one - probably, we can use both
approaches together and refactor or change our pa
19 matches
Mail list logo
