ahoppen 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.
----------------
jansvoboda11 wrote:
> 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.
OK. Makes sense. Thanks.
================
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
----------------
jansvoboda11 wrote:
> ahoppen wrote:
> > Nitpick: `SearchDirs[Idx]` can be simplified to `SearchDir->isFramework()`.
> > Similarly below.
> `SearchDir` will be removed in the following patch: D113676.
Ah, OK. In that case it makes sense to keep using `SearchDirs[Idx]`.
================
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};
----------------
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.
```
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
