[PATCH] D89297: [clangd] Add a TestWorkspace utility

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

thanks this mostly looks good.

can you move implementations from TestWorkspace.h to TestWorkspace.cpp ?



================
Comment at: clang-tools-extra/clangd/unittests/FileIndexTests.cpp:431
+TEST(FileIndexTest, RelationsMultiFile) {
+  TestWorkspace Workspace({{"Base.h", "class Base {};", false},
+                           {"A.cpp", R"cpp(
----------------
nit: if you decide to keep the constructor can you prepend `/*IsMainFile=*/` to last parameters?


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:12
+//
+// The tests can exercise both index- and AST-based operations.
+//
----------------
s/index-/index/
s/AST-based/AST based/


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:32
+public:
+  struct SourceFile {
+    std::string Filename;
----------------
i think it would be better to move this struct to private, and only have addSoruce/addMainFile helpers with comments explaining only TUs rooted at mainfiles will be indexed.

if you prefer to keep this struct then we should probably have comments about it too. especially the `IsMainFile` bit.


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:46
+  void addSource(llvm::StringRef Filename, llvm::StringRef Code) {
+    addInput({std::string(Filename), std::string(Code), false});
+  }
----------------
nit: Filename.str() (same for `Code` and usages in `addMainFile` below.


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:56
+    for (const auto &Input : Inputs) {
+      if (Input.IsMainFile) {
+        TU.Code = Input.Code;
----------------
nit: prefer early exit, `if(!MainFile) continue;`


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:75
+    if (!Input)
+      return llvm::None;
+    TU.Code = Input->Code;
----------------
should this be a failure instead? e.g. `ADD_FAILURE() << "Accessing non-existing file: "  << Filename;`


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:77
+    TU.Code = Input->Code;
+    TU.Filename = std::string(Filename);
+    return TU.build();
----------------
nit: `Input->Filename`


================
Comment at: clang-tools-extra/clangd/unittests/TestWorkspace.h:82
+private:
+  std::vector<SourceFile> Inputs;
+  TestTU TU;
----------------
nit: maybe move filename out of the struct, and keep a map here instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89297/new/

https://reviews.llvm.org/D89297