jkorous marked 6 inline comments as done. jkorous added inline comments.
================ Comment at: clangd/CodeCompleteTests.cpp:2043 +TEST(CompletionTestNoExplicitMembers, Struct) { + clangd::CodeCompleteOptions Opts; ---------------- sammccall wrote: > sammccall wrote: > > (Should this be ImplicitMembers?) > nit: these cases should probably be moved up with the other code completion > ones, and called something like `TEST(CompletionTest, NoExplicitMembers)` or > so for consistency. > > It's OK to have related tests in one test case. > > In fact, this large set of closely-related cases seems like a good place for > Go-style table-driven tests. Ultimately got rid of the name completely. ================ Comment at: clangd/CodeCompleteTests.cpp:2043 +TEST(CompletionTestNoExplicitMembers, Struct) { + clangd::CodeCompleteOptions Opts; ---------------- jkorous wrote: > sammccall wrote: > > sammccall wrote: > > > (Should this be ImplicitMembers?) > > nit: these cases should probably be moved up with the other code completion > > ones, and called something like `TEST(CompletionTest, NoExplicitMembers)` > > or so for consistency. > > > > It's OK to have related tests in one test case. > > > > In fact, this large set of closely-related cases seems like a good place > > for Go-style table-driven tests. > Ultimately got rid of the name completely. I reconsidered the simple yet verbatim approach. ================ Comment at: clangd/CodeCompleteTests.cpp:2054 + + EXPECT_TRUE(ResultsDot.Completions.empty()); + ---------------- sammccall wrote: > EXPECT_THAT(ResultsDot.Completions, IsEmpty()) > > (when the assertion fails, the failure message will print the contents of the > container) I didn't know this. It's pretty neat. Thanks! ================ Comment at: clangd/CodeCompleteTests.cpp:2139 + +TEST(CompletionTestMethodDeclared, Struct) { + clangd::CodeCompleteOptions Opts; ---------------- sammccall wrote: > doesn't this test a strict superset of what CompletionTestNoTestMembers tests? > i.e. this also asserts that the implicit members are not included. > > ISTM we could combine many of these tests (and that in fact many of them are > covered by TestAfterDotCompletion. You are right, I pruned the list of testcases a bit. ================ Comment at: clangd/CodeCompleteTests.cpp:2338 + +TEST(CompletionTestSpecialMethodsDeclared, ExplicitStructTemplateSpecialization) { + clangd::CodeCompleteOptions Opts; ---------------- sammccall wrote: > (I think we could get away with a representative set of cases, rather than > testing the intersection of every feature. e.g. test an explicitly declared > operator= on a struct, but combining that with an explicitly specialized > struct template seems unneccesary) I simplified the whole test and removed some cases that were not really unique. Do you think the rest is still too exhaustive? (Asking before deleting.) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52554 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits