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

[Jan Svoboda via Phabricator via cfe-commits]

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