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