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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
