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
21

Is this FIXME obsolete now?

clang/unittests/AST/ASTImporterVisibilityTest.cpp
38

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

50

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

140

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

158

This should fit a single line.

191

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
21

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

clang/unittests/AST/ASTImporterVisibilityTest.cpp
50

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.