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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
