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

[Alex Hoppen via Phabricator via cfe-commits]

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