ilya-biryukov added a comment. Neat, thanks for the cleanup! A few suggestions from me, no major comments.
================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.cpp:74 + return true; + llvm::consumeError(PrepareResult.takeError()); + return false; ---------------- Maybe print the input in case of failure? ================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:9 + +#include "TestTU.h" +#include "gtest/gtest.h" ---------------- NIT: add an include guard ================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:44 + // Snippet is an expression. + Expression, + }; ---------------- I wonder whether we could use `Function` and get rid of 'expression' mode completely? WDYT? One can easily turn an expression into a statement by adding a semicolon. ================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:86 + for (const auto &Case : expandCases(MarkedCode)) \ + EXPECT_THAT(Case, isAvailable()) + ---------------- NIT: add a level of indent ================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:86 + for (const auto &Case : expandCases(MarkedCode)) \ + EXPECT_THAT(Case, isAvailable()) + ---------------- ilya-biryukov wrote: > NIT: add a level of indent NIT: fully-qualify in a macro `::clang::clangd::TweakTest::isAvailable()` ================ Comment at: clang-tools-extra/clangd/unittests/TweakTesting.h:90 + for (const auto &Case : expandCases(MarkedCode)) \ + EXPECT_THAT(Case, ::testing::Not(isAvailable())) + ---------------- NIT: add a level of indent ================ Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:36 +// TODO(sammccall): migrate the rest of the tests to use TweakTesting.h and +// remove these helpers. ---------------- NIT: s/TODO/FIXME the latter is used more widely in clangd codebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65525/new/ https://reviews.llvm.org/D65525 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
