This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Separate unittest files
ClosedPublic

Authored by martong on May 10 2019, 8:02 AM.

Event Timeline

martong created this revision.May 10 2019, 8:02 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
a_sidorin accepted this revision.May 11 2019, 10:43 AM

Hi Gabor,
I like this change! LGTM, just a few nits.

clang/unittests/AST/ASTImporterGenericRedeclTest.cpp
20

Is this FIXME obsolete now?

clang/unittests/AST/ASTImporterVisibilityTest.cpp
37

Can we use StringRef (or, at least const auto *)?

49

Double spaces in the end of sentences look a bit strange.

139

Optional: F0/F1 (where F stands for "function", I guess) can be replaced with D0/D1 (for decl) since the code is generic.

157

This should fit a single line.

190

const?

This revision is now accepted and ready to land.May 11 2019, 10:43 AM
martong marked 8 inline comments as done.May 13 2019, 2:46 AM

Thanks for the review Alexei!

clang/unittests/AST/ASTImporterGenericRedeclTest.cpp
20

Yes, that's correct, thank you for finding it!

clang/unittests/AST/ASTImporterVisibilityTest.cpp
49

Ok I removed them. (Note, this is the result of some autoformatting option in vim, could not discover which specific setting causes the insertion of double spaces.)

martong updated this revision to Diff 199225.May 13 2019, 2:47 AM
martong marked 2 inline comments as done.
  • Address Alexei's comments
This revision was automatically updated to reflect the committed changes.