[PATCH] D116750: [clang][lex] Keep references to `DirectoryLookup` objects up-to-date

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 4 inline comments as done.
jansvoboda11 added a comment.

Thanks for the feedback!



================
Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:276
+    std::vector<bool> ExpectedSearchDirUsageAfterM2{false, true, false};
+    EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+    std::vector<bool> ExpectedUserEntryUsageAfterM2{false, true, false};
----------------
ahoppen wrote:
> jansvoboda11 wrote:
> > ahoppen wrote:
> > > Wouldn’t it be cleaner to just check that `UsedSearchDirs` only contains a single element and that it’s name is `/M2`? In that case we could also remove `getSearchDirUsage`.
> > Maybe I'm misunderstanding you, but I don't think so. We'd still need accessor for `HeaderSearch::UsedSearchDirs` and we don't have the expected `DirectoryLookup *` lying around, making matching more cumbersome:
> > 
> > ```
> >     const llvm::DenseSet<DirectoryLookup *> &UsedSearchDirs =
> >         Search.getUsedSearchDirs();
> >     EXPECT_EQ(UsedSearchDirs.size(), 2);
> >     EXPECT_EQ(1, llvm::count_if(UsedSearchDirs, [](const auto *SearchDir) {
> >                 return SearchDir->getName() == "/M1";
> >               }));
> >     EXPECT_EQ(1, llvm::count_if(UsedSearchDirs, [](const auto *SearchDir) {
> >                 return SearchDir->getName() == "/M2";
> >               }));
> > ```
> > 
> > or
> > 
> > ```
> >     llvm::DenseSet<std::string> UsedSearchDirsStr;
> >     for (const auto *SearchDir : Search.getUsedSearchDirs())
> >       UsedSearchDirsStr.insert(SearchDir->getName());
> >     EXPECT_EQ(UsedSearchDirsStr, (llvm::DenseSet<std::string>{"/M1", "/M2"}));
> > ```
> > 
> > I think having bit-vector, whose indices correspond to the directory names (`"/M{i}"`), and using `operator==` for matching is simpler.
> > 
> > Let me know if you had something else in mind.
> I just don’t like the bit-vectors and basically thought of the second option you were suggesting, but maybe that’s really just personal taste. If you’d like to keep the bit-vector, could you change the comment of `getSearchDirUsage` to something like 
> ```
> /// Return a vector of length \c SearchDirs.size() that indicates for each search directory whether it was used.
> ```
I've updated documentation for `HeaderSearch::getSearchDirUsage`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116750