[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:415
   StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
-  const DirectoryLookup *FromDir, const DirectoryLookup **CurDir,
+  maybe_search_dir_iterator FromDir, maybe_search_dir_iterator *CurDir,
   ArrayRef> Includers,

jansvoboda11 wrote:
> This is the interesting change in the latest revision. Callers of this 
> function performed pointer arithmetics on `const DirectoryLookup *`, 
> essentially treating it as an iterator. Since `DirectoryLookup` objects are 
> no longer stored in contiguous memory, that's unsafe. I'm using an iterator 
> to allow doing that safely and using `llvm::Optional` to provide the 
> nullability clients take advantage of.
> 
> I don't love the long name that causes a bunch of formatting changes. Maybe 
> it's worth extracting it out of `HeaderSearch` and giving it shorter name. I 
> also don't love that the API is not the same as for `const DirectoryLookup 
> *`, leading to some "unnecessary" changes (e.g. `CurDir = nullptr` -> 
> `CurDir.reset()`). I think it might be worth creating custom iterator type 
> that would have the original API, but would behave correctly with the new 
> memory setup.
Resolved in the latest revision by introducing D117566.


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 400852.
jansvoboda11 added a comment.

Rebase on top of D117566 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116750/new/

https://reviews.llvm.org/D116750

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -23,6 +23,12 @@
 namespace clang {
 namespace {
 
+static std::shared_ptr createTargetOptions() {
+  auto TargetOpts = std::make_shared();
+  TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
+  return TargetOpts;
+}
+
 // The test fixture.
 class HeaderSearchTest : public ::testing::Test {
 protected:
@@ -30,12 +36,10 @@
   : VFS(new llvm::vfs::InMemoryFileSystem), FileMgr(FileMgrOpts, VFS),
 DiagID(new DiagnosticIDs()),
 Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
-SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions),
+SourceMgr(Diags, FileMgr), TargetOpts(createTargetOptions()),
+Target(TargetInfo::CreateTargetInfo(Diags, TargetOpts)),
 Search(std::make_shared(), SourceMgr, Diags,
-   LangOpts, Target.get()) {
-TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
-  }
+   LangOpts, Target.get()) {}
 
   void addSearchDir(llvm::StringRef Dir) {
 VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
@@ -55,6 +59,27 @@
 Search.AddSystemSearchPath(DL);
   }
 
+  void setSearchDirs(llvm::ArrayRef QuotedDirs,
+ llvm::ArrayRef AngledDirs) {
+auto AddPath = [&](StringRef Dir, bool IsAngled) {
+  VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+   /*Group=*/None, llvm::sys::fs::file_type::directory_file);
+  auto Group = IsAngled ? frontend::IncludeDirGroup::Angled
+: frontend::IncludeDirGroup::Quoted;
+  Search.getHeaderSearchOpts().AddPath(Dir, Group,
+   /*IsFramework=*/false,
+   /*IgnoreSysRoot=*/true);
+};
+
+for (llvm::StringRef Dir : QuotedDirs)
+  AddPath(Dir, /*IsAngled=*/false);
+for (llvm::StringRef Dir : AngledDirs)
+  AddPath(Dir, /*IsAngled=*/true);
+
+clang::ApplyHeaderSearchOptions(Search, Search.getHeaderSearchOpts(),
+LangOpts, Target->getTriple());
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
 std::unique_ptr Buf,
 bool isAngled = false) {
@@ -71,6 +96,17 @@
 Search.AddSearchPath(DL, isAngled);
   }
 
+  void createModule(StringRef Mod) {
+std::string ModDir = ("/" + Mod).str();
+std::string ModHeader = (Mod + ".h").str();
+VFS->addFile(
+ModDir + "/module.modulemap", 0,
+llvm::MemoryBuffer::getMemBufferCopy(
+("module " + Mod + " { header \"" + ModHeader + "\" }").str()));
+VFS->addFile(ModDir + "/" + ModHeader, 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -254,5 +290,31 @@
   EXPECT_EQ(FI->Framework.str(), "Foo");
 }
 
+TEST_F(HeaderSearchTest, SearchPathUsage) {
+  Search.getHeaderSearchOpts().ImplicitModuleMaps = true;
+
+  setSearchDirs(/*QuotedDirs=*/{"/M0"}, /*AngledDirs=*/{"/M2", "/M3"});
+  createModule("M0");
+  createModule("M2");
+  createModule("M3");
+
+  {
+Module *M2 = Search.lookupModule("M2");
+EXPECT_NE(M2, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+
+  addSearchDir("/M1");
+  createModule("M1");
+
+  {
+Module *M1 = Search.lookupModule("M1");
+EXPECT_NE(M1, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+}
+
 } // namespace
 } // namespace clang
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -79,6 +79,10 @@
 
 ExternalHeaderFileInfoSource::~ExternalHeaderFileInfoSource() = default;
 
+const DirectoryLookup ::operator*() const {
+  return *HS->SearchDirs[Idx];
+}
+
 HeaderSearch::HeaderSearch(std::shared_ptr HSOpts,
SourceManager , DiagnosticsEngine ,
const LangOptions ,
@@ -115,19 +119,24 @@
 llvm::DenseMap searchDirToHSEntry) {
   assert(angledDirIdx <= systemDirIdx && systemDirIdx <= dirs.size() &&
   

[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:415
   StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
-  const DirectoryLookup *FromDir, const DirectoryLookup **CurDir,
+  maybe_search_dir_iterator FromDir, maybe_search_dir_iterator *CurDir,
   ArrayRef> Includers,

This is the interesting change in the latest revision. Callers of this function 
performed pointer arithmetics on `const DirectoryLookup *`, essentially 
treating it as an iterator. Since `DirectoryLookup` objects are no longer 
stored in contiguous memory, that's unsafe. I'm using an iterator to allow 
doing that safely and using `llvm::Optional` to provide the nullability clients 
take advantage of.

I don't love the long name that causes a bunch of formatting changes. Maybe 
it's worth extracting it out of `HeaderSearch` and giving it shorter name. I 
also don't love that the API is not the same as for `const DirectoryLookup *`, 
leading to some "unnecessary" changes (e.g. `CurDir = nullptr` -> 
`CurDir.reset()`). I think it might be worth creating custom iterator type that 
would have the original API, but would behave correctly with the new memory 
setup.


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 399981.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

Replace external uses of `const DirectoryLookup *` by an optional iterator. 
That's how they get treated by callers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116750/new/

https://reviews.llvm.org/D116750

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/Preprocessor.h
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/lib/Interpreter/IncrementalParser.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Lex/PPMacroExpansion.cpp
  clang/lib/Lex/Pragma.cpp
  clang/lib/Lex/Preprocessor.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -23,6 +23,12 @@
 namespace clang {
 namespace {
 
+static std::shared_ptr createTargetOptions() {
+  auto TargetOpts = std::make_shared();
+  TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
+  return TargetOpts;
+}
+
 // The test fixture.
 class HeaderSearchTest : public ::testing::Test {
 protected:
@@ -30,12 +36,10 @@
   : VFS(new llvm::vfs::InMemoryFileSystem), FileMgr(FileMgrOpts, VFS),
 DiagID(new DiagnosticIDs()),
 Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
-SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions),
+SourceMgr(Diags, FileMgr), TargetOpts(createTargetOptions()),
+Target(TargetInfo::CreateTargetInfo(Diags, TargetOpts)),
 Search(std::make_shared(), SourceMgr, Diags,
-   LangOpts, Target.get()) {
-TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
-  }
+   LangOpts, Target.get()) {}
 
   void addSearchDir(llvm::StringRef Dir) {
 VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
@@ -55,6 +59,27 @@
 Search.AddSystemSearchPath(DL);
   }
 
+  void setSearchDirs(llvm::ArrayRef QuotedDirs,
+ llvm::ArrayRef AngledDirs) {
+auto AddPath = [&](StringRef Dir, bool IsAngled) {
+  VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+   /*Group=*/None, llvm::sys::fs::file_type::directory_file);
+  auto Group = IsAngled ? frontend::IncludeDirGroup::Angled
+: frontend::IncludeDirGroup::Quoted;
+  Search.getHeaderSearchOpts().AddPath(Dir, Group,
+   /*IsFramework=*/false,
+   /*IgnoreSysRoot=*/true);
+};
+
+for (llvm::StringRef Dir : QuotedDirs)
+  AddPath(Dir, /*IsAngled=*/false);
+for (llvm::StringRef Dir : AngledDirs)
+  AddPath(Dir, /*IsAngled=*/true);
+
+clang::ApplyHeaderSearchOptions(Search, Search.getHeaderSearchOpts(),
+LangOpts, Target->getTriple());
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
 std::unique_ptr Buf,
 bool isAngled = false) {
@@ -71,6 +96,17 @@
 Search.AddSearchPath(DL, isAngled);
   }
 
+  void createModule(StringRef Mod) {
+std::string ModDir = ("/" + Mod).str();
+std::string ModHeader = (Mod + ".h").str();
+VFS->addFile(
+ModDir + "/module.modulemap", 0,
+llvm::MemoryBuffer::getMemBufferCopy(
+("module " + Mod + " { header \"" + ModHeader + "\" }").str()));
+VFS->addFile(ModDir + "/" + ModHeader, 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -239,7 +275,7 @@
 
   bool IsMapped = false;
   auto FoundFile = Search.LookupFile(
-  "Foo/Foo.h", SourceLocation(), /*isAngled=*/true, /*FromDir=*/nullptr,
+  "Foo/Foo.h", SourceLocation(), /*isAngled=*/true, /*FromDir=*/None,
   /*CurDir=*/nullptr, /*Includers=*/{}, /*SearchPath=*/nullptr,
   /*RelativePath=*/nullptr, /*RequestingModule=*/nullptr,
   /*SuggestedModule=*/nullptr, ,
@@ -254,5 +290,31 @@
   EXPECT_EQ(FI->Framework.str(), "Foo");
 }
 
+TEST_F(HeaderSearchTest, SearchPathUsage) {
+  Search.getHeaderSearchOpts().ImplicitModuleMaps = true;
+
+  setSearchDirs(/*QuotedDirs=*/{"/M0"}, /*AngledDirs=*/{"/M2", "/M3"});
+  createModule("M0");
+  createModule("M2");
+  createModule("M3");
+
+  {
+Module *M2 = Search.lookupModule("M2");
+EXPECT_NE(M2, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+
+  addSearchDir("/M1");
+  createModule("M1");
+
+  {
+Module *M1 = Search.lookupModule("M1");
+EXPECT_NE(M1, 

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 reopened this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

I'll need to figure out how to efficiently replace the pointer arithmetic 
performed by `Preprocessor::LookupFile`, or go back to my first solution: 
keeping `std::vector` and updating indices in 
`AddSearchPath`...


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

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Great catch and thanks for the revert!


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thank you!

I'm pretty sure it's this commit that causes the failure.

I now see that `HeaderSearch::LookupFile` callers perform pointer arithmetics 
on the out-parameter `const DirectoryLookup *`. They rely on the fact 
that all `DirectoryLookup` instances are in contiguous memory. That was true 
when they were stored in `std::vector`, but since they are 
allocated in `llvm::SpecificBumpPtrAllocator` after this 
patch, that assumption is no longer true.

I have reverted the patch for now and will look into fixing this.


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

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D116750#3240680 , @jansvoboda11 
wrote:

> In D116750#3240363 , @carlosgalvezp 
> wrote:
>
>> Hi @jansvoboda11 !
>>
>> I believe this commit is the root cause of this issue: 
>> https://github.com/llvm/llvm-project/issues/53161
>>
>> Believe it or not, I was (un)lucky enough to run into it. I have bisected 
>> the repo and my tests indicate that this commit is the offender. Do you have 
>> any obvious idea what could be the problem?
>>
>> I'm suspecting this is the offender, since it's related to `#include_next` 
>> which is where I get the error:
>> https://github.com/llvm/llvm-project/commit/8503c688d555014b88849e933bf096035a351586#diff-fb96fa41fef79b66ab53ac1d845ab467defc41c2639e9b0cd60b471b04f898cdR981
>> To summarize, clang cannot find `/usr/include` only if I include exactly 511 
>> directories via `-I`. If I include 510 or 512, then it works fine. I will 
>> test if the same happens with smaller powers of two. I have a test script 
>> that I can share if you want to test it yourself.
>>
>> EDIT: the same happen when including exactly 255 directories. It does not 
>> happen for 127 and below. It does happen for 1023 and above.
>
> Hi, thanks for reporting this. The reproducer script would be helpful. 
> Looking at the code, I don't see anything obviously wrong.

Sure, I attach it do this message. It assumes you run from a place from which 
you can run `./build/bin/clang`.F21622105: test.py 


Let me know if I can provide anything else to help! It might not be this commit 
in particular, perhaps the problem is in some allocator class merged earlier.


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D116750#3240363 , @carlosgalvezp 
wrote:

> Hi @jansvoboda11 !
>
> I believe this commit is the root cause of this issue: 
> https://github.com/llvm/llvm-project/issues/53161
>
> Believe it or not, I was (un)lucky enough to run into it. I have bisected the 
> repo and my tests indicate that this commit is the offender. Do you have any 
> obvious idea what could be the problem?
>
> I'm suspecting this is the offender, since it's related to `#include_next` 
> which is where I get the error:
> https://github.com/llvm/llvm-project/commit/8503c688d555014b88849e933bf096035a351586#diff-fb96fa41fef79b66ab53ac1d845ab467defc41c2639e9b0cd60b471b04f898cdR981
> To summarize, clang cannot find `/usr/include` only if I include exactly 511 
> directories via `-I`. If I include 510 or 512, then it works fine. I will 
> test if the same happens with smaller powers of two. I have a test script 
> that I can share if you want to test it yourself.
>
> EDIT: the same happen when including exactly 255 directories. It does not 
> happen for 127 and below. It does happen for 1023 and above.

Hi, thanks for reporting this. The reproducer script would be helpful. Looking 
at the code, I don't see anything obviously wrong.


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

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Hi @jansvoboda11 !

I believe this commit is the root cause of this issue: 
https://github.com/llvm/llvm-project/issues/53161

Believe it or not, I was lucky enough to run into it. I have bisected the repo 
and my tests indicate that this commit is the offender. Do you have any obvious 
idea what could be the problem?

To summarize, clang cannot find /usr/include only if I include exactly 511 
directories via `-I`. If I include 510 or 512, then it works fine. I will test 
if the same happens with smaller powers of two. I have a test script that I can 
share if you want to test it yourself.


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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> That's referring to the fact that once we allocate new `DirectoryLookup` with 
> `SpecificBumpPtrAllocator`, address of that object won't change (unlike with 
> `std::vector`). This means that we can take the address and use it without 
> worrying about invalidation.

Ah, "stable" made me think of iteration order, but that's not what you meant. 
Makes sense, thanks!


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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D116750#3234261 , @thakis wrote:

> 1. `which is stable thanks to the bump-ptr-allocation strategy.` I don't 
> understand this. In each slab, that's true, but why is it true between 
> objects allocated in different slabs?

That's referring to the fact that once we allocate new `DirectoryLookup` with 
`SpecificBumpPtrAllocator`, address of that object won't change (unlike with 
`std::vector`). This means that we can take the address and use it without 
worrying about invalidation.

> 2. This increases numbers of TUs compiled for LexTests by over 10%. Is there 
> no way around that Frontend dep?

I'll look into moving `clang::ApplyHeaderSearchOptions` from `Frontend` into 
`Lex`.


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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

1. `which is stable thanks to the bump-ptr-allocation strategy.` I don't 
understand this. In each slab, that's true, but why is it true between objects 
allocated in different slabs?
2. This increases numbers of TUs compiled for LexTests by over 10%. Is there no 
way around that Frontend dep?


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

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

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
jansvoboda11 marked an inline comment as done.
Closed by commit rG8503c688d555: [clang][lex] Keep references to 
`DirectoryLookup` objects up-to-date (authored by jansvoboda11).

Changed prior to commit:
  https://reviews.llvm.org/D116750?vs=398917=398932#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116750/new/

https://reviews.llvm.org/D116750

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -15,6 +15,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
+#include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
@@ -24,6 +25,12 @@
 namespace clang {
 namespace {
 
+static std::shared_ptr createTargetOptions() {
+  auto TargetOpts = std::make_shared();
+  TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
+  return TargetOpts;
+}
+
 // The test fixture.
 class HeaderSearchTest : public ::testing::Test {
 protected:
@@ -31,12 +38,10 @@
   : VFS(new llvm::vfs::InMemoryFileSystem), FileMgr(FileMgrOpts, VFS),
 DiagID(new DiagnosticIDs()),
 Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
-SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions),
+SourceMgr(Diags, FileMgr), TargetOpts(createTargetOptions()),
+Target(TargetInfo::CreateTargetInfo(Diags, TargetOpts)),
 Search(std::make_shared(), SourceMgr, Diags,
-   LangOpts, Target.get()) {
-TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
-  }
+   LangOpts, Target.get()) {}
 
   void addSearchDir(llvm::StringRef Dir) {
 VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
@@ -56,6 +61,27 @@
 Search.AddSystemSearchPath(DL);
   }
 
+  void setSearchDirs(llvm::ArrayRef QuotedDirs,
+ llvm::ArrayRef AngledDirs) {
+auto AddPath = [&](StringRef Dir, bool IsAngled) {
+  VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+   /*Group=*/None, llvm::sys::fs::file_type::directory_file);
+  auto Group = IsAngled ? frontend::IncludeDirGroup::Angled
+: frontend::IncludeDirGroup::Quoted;
+  Search.getHeaderSearchOpts().AddPath(Dir, Group,
+   /*IsFramework=*/false,
+   /*IgnoreSysRoot=*/true);
+};
+
+for (llvm::StringRef Dir : QuotedDirs)
+  AddPath(Dir, /*IsAngled=*/false);
+for (llvm::StringRef Dir : AngledDirs)
+  AddPath(Dir, /*IsAngled=*/true);
+
+clang::ApplyHeaderSearchOptions(Search, Search.getHeaderSearchOpts(),
+LangOpts, Target->getTriple());
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
 std::unique_ptr Buf,
 bool isAngled = false) {
@@ -72,6 +98,17 @@
 Search.AddSearchPath(DL, isAngled);
   }
 
+  void createModule(StringRef Mod) {
+std::string ModDir = ("/" + Mod).str();
+std::string ModHeader = (Mod + ".h").str();
+VFS->addFile(
+ModDir + "/module.modulemap", 0,
+llvm::MemoryBuffer::getMemBufferCopy(
+("module " + Mod + " { header \"" + ModHeader + "\" }").str()));
+VFS->addFile(ModDir + "/" + ModHeader, 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -256,5 +293,31 @@
   EXPECT_EQ(FI->Framework.str(), "Foo");
 }
 
+TEST_F(HeaderSearchTest, SearchPathUsage) {
+  Search.getHeaderSearchOpts().ImplicitModuleMaps = true;
+
+  setSearchDirs(/*QuotedDirs=*/{"/M0"}, /*AngledDirs=*/{"/M2", "/M3"});
+  createModule("M0");
+  createModule("M2");
+  createModule("M3");
+
+  {
+Module *M2 = Search.lookupModule("M2");
+EXPECT_NE(M2, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+
+  addSearchDir("/M1");
+  createModule("M1");
+
+  {
+Module *M1 = Search.lookupModule("M1");
+EXPECT_NE(M1, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+}
+
 } // namespace
 } // namespace clang
Index: clang/unittests/Lex/CMakeLists.txt

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

[Jan Svoboda via Phabricator via cfe-commits]

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 ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector 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  =
> > 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 UsedSearchDirsStr;
> > for (const auto *SearchDir : Search.getUsedSearchDirs())
> >   UsedSearchDirsStr.insert(SearchDir->getName());
> > EXPECT_EQ(UsedSearchDirsStr, (llvm::DenseSet{"/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

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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 398917.
jansvoboda11 added a comment.

Update documentation, inline variables in test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116750/new/

https://reviews.llvm.org/D116750

Files:
  clang/include/clang/Lex/HeaderSearch.h
  clang/lib/Lex/HeaderSearch.cpp
  clang/unittests/Lex/CMakeLists.txt
  clang/unittests/Lex/HeaderSearchTest.cpp

Index: clang/unittests/Lex/HeaderSearchTest.cpp
===
--- clang/unittests/Lex/HeaderSearchTest.cpp
+++ clang/unittests/Lex/HeaderSearchTest.cpp
@@ -15,6 +15,7 @@
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
+#include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
@@ -24,6 +25,12 @@
 namespace clang {
 namespace {
 
+static std::shared_ptr createTargetOptions() {
+  auto TargetOpts = std::make_shared();
+  TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
+  return TargetOpts;
+}
+
 // The test fixture.
 class HeaderSearchTest : public ::testing::Test {
 protected:
@@ -31,12 +38,10 @@
   : VFS(new llvm::vfs::InMemoryFileSystem), FileMgr(FileMgrOpts, VFS),
 DiagID(new DiagnosticIDs()),
 Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
-SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions),
+SourceMgr(Diags, FileMgr), TargetOpts(createTargetOptions()),
+Target(TargetInfo::CreateTargetInfo(Diags, TargetOpts)),
 Search(std::make_shared(), SourceMgr, Diags,
-   LangOpts, Target.get()) {
-TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
-Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
-  }
+   LangOpts, Target.get()) {}
 
   void addSearchDir(llvm::StringRef Dir) {
 VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
@@ -47,6 +52,27 @@
 Search.AddSearchPath(DL, /*isAngled=*/false);
   }
 
+  void setSearchDirs(llvm::ArrayRef QuotedDirs,
+ llvm::ArrayRef AngledDirs) {
+auto AddPath = [&](StringRef Dir, bool IsAngled) {
+  VFS->addFile(Dir, 0, llvm::MemoryBuffer::getMemBuffer(""), /*User=*/None,
+   /*Group=*/None, llvm::sys::fs::file_type::directory_file);
+  auto Group = IsAngled ? frontend::IncludeDirGroup::Angled
+: frontend::IncludeDirGroup::Quoted;
+  Search.getHeaderSearchOpts().AddPath(Dir, Group,
+   /*IsFramework=*/false,
+   /*IgnoreSysRoot=*/true);
+};
+
+for (llvm::StringRef Dir : QuotedDirs)
+  AddPath(Dir, /*IsAngled=*/false);
+for (llvm::StringRef Dir : AngledDirs)
+  AddPath(Dir, /*IsAngled=*/true);
+
+clang::ApplyHeaderSearchOptions(Search, Search.getHeaderSearchOpts(),
+LangOpts, Target->getTriple());
+  }
+
   void addHeaderMap(llvm::StringRef Filename,
 std::unique_ptr Buf,
 bool isAngled = false) {
@@ -63,6 +89,17 @@
 Search.AddSearchPath(DL, isAngled);
   }
 
+  void createModule(StringRef Mod) {
+std::string ModDir = ("/" + Mod).str();
+std::string ModHeader = (Mod + ".h").str();
+VFS->addFile(
+ModDir + "/module.modulemap", 0,
+llvm::MemoryBuffer::getMemBufferCopy(
+("module " + Mod + " { header \"" + ModHeader + "\" }").str()));
+VFS->addFile(ModDir + "/" + ModHeader, 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+  }
+
   IntrusiveRefCntPtr VFS;
   FileSystemOptions FileMgrOpts;
   FileManager FileMgr;
@@ -224,5 +261,31 @@
   EXPECT_EQ(FI->Framework.str(), "Foo");
 }
 
+TEST_F(HeaderSearchTest, SearchPathUsage) {
+  Search.getHeaderSearchOpts().ImplicitModuleMaps = true;
+
+  setSearchDirs(/*QuotedDirs=*/{"/M0"}, /*AngledDirs=*/{"/M2", "/M3"});
+  createModule("M0");
+  createModule("M2");
+  createModule("M3");
+
+  {
+Module *M2 = Search.lookupModule("M2");
+EXPECT_NE(M2, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+
+  addSearchDir("/M1");
+  createModule("M1");
+
+  {
+Module *M1 = Search.lookupModule("M1");
+EXPECT_NE(M1, nullptr);
+EXPECT_EQ(Search.getSearchDirUsage(), (std::vector{0, 1, 1, 0}));
+EXPECT_EQ(Search.computeUserEntryUsage(), (std::vector{0, 1, 0}));
+  }
+}
+
 } // namespace
 } // namespace clang
Index: clang/unittests/Lex/CMakeLists.txt
===
--- clang/unittests/Lex/CMakeLists.txt
+++ clang/unittests/Lex/CMakeLists.txt
@@ -15,6 +15,7 @@
   PRIVATE
   clangAST
   clangBasic
+  clangFrontend
   clangLex
   clangParse
   clangSema
Index: 

[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 SearchDirs;
-  /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
-  /// been successfully used to lookup a file.
-  std::vector SearchDirsUsage;
+  std::vector 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 ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector 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  =
> 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 UsedSearchDirsStr;
> for (const auto *SearchDir : Search.getUsedSearchDirs())
>   UsedSearchDirsStr.insert(SearchDir->getName());
> EXPECT_EQ(UsedSearchDirsStr, (llvm::DenseSet{"/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

[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 SearchDirs;
-  /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
-  /// been successfully used to lookup a file.
-  std::vector SearchDirsUsage;
+  std::vector 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 ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector 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  =
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 UsedSearchDirsStr;
for (const auto *SearchDir : Search.getUsedSearchDirs())
  UsedSearchDirsStr.insert(SearchDir->getName());
EXPECT_EQ(UsedSearchDirsStr, (llvm::DenseSet{"/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

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

[Alex Hoppen via Phabricator via cfe-commits]

ahoppen added a comment.

I like this a lot better. Some comments inline.




Comment at: clang/include/clang/Lex/HeaderSearch.h:179
   /// directory is suppressed.
-  std::vector SearchDirs;
-  /// Whether the DirectoryLookup at the corresponding index in SearchDirs has
-  /// been successfully used to lookup a file.
-  std::vector SearchDirsUsage;
+  std::vector SearchDirs;
+  /// Set of SearchDirs that have been successfully used to lookup a file.

I haven’t tried it but is there a particular reason why this can’t be a `const 
DirectoryLookup *`?



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

Nitpick: `SearchDirs[Idx]` can be simplified to `SearchDir->isFramework()`. 
Similarly below.



Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:276
+std::vector ExpectedSearchDirUsageAfterM2{false, true, false};
+EXPECT_EQ(Search.getSearchDirUsage(), ExpectedSearchDirUsageAfterM2);
+std::vector ExpectedUserEntryUsageAfterM2{false, true, false};

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`.


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