[PATCH] D54999: [clangd] Populate include graph during static indexing action.

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Nov 30 02:15:37 PST 2018


ilya-biryukov added inline comments.


================
Comment at: unittests/clangd/IndexActionTests.cpp:168
+  std::string MainFilePath = testPath("main.cpp");
+  std::pair<std::string, std::string> CommonHeader = {testPath("common.h"),
+                                                      R"cpp(
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Maybe have two variables instead? Would format better and `HeaderPath` is arguably more readable than `Header.first`
> Is it that important?
I think it is of minor importance.

Creating a pair that is deconstructed at every use site anyway hurts readability.
If the purpose is grouping the variables together, putting them close to each other and giving them similar names solves the same purpose.

I would also expect the raw strings to format better and not have a very long indent if we put them into separate variables.


================
Comment at: unittests/clangd/IndexActionTests.cpp:225
+  ASSERT_TRUE(IndexFile.Sources);
+  auto Nodes = toMap(*IndexFile.Sources);
+
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > Why not `auto Nodes = toMap(*runIndexingAction(MainFilePath).Sources)`?
> > Optional has an assertion, so we'll catch empty results anyway, no need to make the test more verbose
> because toMap doesn't modify the contents of the nodes(to make sure we don't change behaviour during the process), and all the strings are still referring to the keys of the graph. Therefore the graph needs to be kept alive.
I missed this. It makes sense, thanks for clarifying. It's ok to have the boilerplate then.
Would still remove `ASSERT_TRUE(IndexFile.Sources)`, asserts in optional will catch those anyway. But up to you.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999





More information about the cfe-commits mailing list