jansvoboda11 added inline comments.
================
Comment at: clang/include/clang/Lex/HeaderSearch.h:179
/// directory is suppressed.
- std::vector<DirectoryLookup> SearchDirs;
- /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
- /// been successfully used to lookup a file.
- std::vector<bool> SearchDirsUsage;
+ std::vector<DirectoryLookup *> SearchDirs;
+ /// Set of SearchDirs that have been successfully used to lookup a file.
----------------
ahoppen wrote:
> I haven’t tried it but is there a particular reason why this can’t be a
> `const DirectoryLookup *`?
While iterating over `SearchDirs`, the elements can be passed to
`HeaderSearch::loadSubdirectoryModuleMaps` that mutates them.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:323
+
+ if (SearchDirs[Idx]->isFramework()) {
// Search for or infer a module map for a framework. Here we use
----------------
ahoppen wrote:
> Nitpick: `SearchDirs[Idx]` can be simplified to `SearchDir->isFramework()`.
> Similarly below.
`SearchDir` will be removed in the following patch: D113676.
================
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:
> 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.
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
