Page MenuHomePhabricator

[SyntaxTree] Test the List API
ClosedPublic

Authored by eduucaldas on Sep 17 2020, 9:48 AM.

Diff Detail

Event Timeline

eduucaldas created this revision.Sep 17 2020, 9:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 9:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
eduucaldas requested review of this revision.Sep 17 2020, 9:48 AM

Apply arc lint patch

  • [SyntaxTree] Split TreeTest and ListTest testing fixtures.

I made a separate class for the tests on Lists, as it didn't share any methods with the tests for Trees. What do you think about that?

Should I also put the tests for lists in a different file, even though TreeTest.cpp could mean test Tree.h?

gribozavr2 added inline comments.Sep 22 2020, 6:12 AM
clang/include/clang/Tooling/Syntax/Tree.h
224

I'd slightly prefer "null" b/c "nul" refers to the ASCII character. Feel free to add more spaces to make columns line up :)

clang/unittests/Tooling/Syntax/TreeTest.cpp
130
155

Would llvm::interleaveComma help?

331

All of the tests above are testing getElementsAsNodesAndDelimiters -- could you put that into the test names? Or do you plan adding more assertions in the end? Or is that the only meaningful assertion? (I think not, we could also test the low-level non-typed function at least.)

eduucaldas marked 3 inline comments as done.

Answer code review.

eduucaldas marked an inline comment as done.Sep 22 2020, 9:23 AM
eduucaldas added inline comments.
clang/include/clang/Tooling/Syntax/Tree.h
224

Thanks for providing my solution to my crazyness ^^

gribozavr2 accepted this revision.Sep 22 2020, 9:31 AM
This revision is now accepted and ready to land.Sep 22 2020, 9:31 AM
This revision was automatically updated to reflect the committed changes.