[clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile

[jansvoboda11]

Summary

This patch changes the argument type to HeaderSearch::LookupFile() from const DirectoryEntry * to DirectoryEntryRef in order to remove some calls to the deprecated DirectoryEntry::getName().

Depends on D127660.

[bnbarham]

clang/lib/Frontend/FrontendAction.cpp

828–830

Worth adding an assert here? It'd be nice to cleanup the CWD handling in FileManager at some point.

clang/lib/Lex/PPDirectives.cpp

938

Could we change this to a FileEntryRef as well, or would you prefer to do that later? It'll touch basically all the same places again (eg. each parameter above).

[bnbarham]

Failing test seems to be because . is turned into the full path at some point. It's possible that this is because getFileRef returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing clang-apply-replacements and then change the Status.getName() == Filename to only check whether it's an externally mapped path.

[jansvoboda11]

Fix the Modules/filename.cpp test

[jansvoboda11]

The failing test was a fun one to debug. It didn't test what it intended to. It wanted to check that header referenced from module map is found via a search path, but the command line was constructed in such way that the header was found relative to the includer. The test relied on the FileManager hackery that mutates FileEntry objects. Now that we call SourceMgr.getFileEntryRefForID(), we don't see effects of this hack. I ended up only fixing the test, since removing the FileManager hack safely probably requires us to remove all remaining {File,Directory}Entry::getName() usages.

LMK if you have more feedback.

clang/lib/Frontend/FrontendAction.cpp

828–830

Assert that Dir is not empty? Seems overly defensive to me, what would be the benefit of that? Catching if the working directory was yanked from underneath Clang?

clang/lib/Lex/PPDirectives.cpp

938

I'd prefer to do it separately for smaller diff and easier bisection. LMK if you have strong preference for doing it in the same commit.

[jansvoboda11]

Match Windows \\ path separators too

[bnbarham]

clang/lib/Frontend/FrontendAction.cpp

828–830

Not sure what I was thinking here - the assertion itself would be pointless, since *Dir will assert non-empty anyway. I imagine this was more about how we're grabbing an optional and not checking it, which IMO is often an anti pattern. What do you want to happen if it is somehow empty? Couldn't we just not do the push_back?

Also could use emplace_back(nullptr, *Dir) instead, not that it really matters.

clang/lib/Lex/PPDirectives.cpp

938

Not really, just seemed like it would hit all the same lines to me so it'd be roughly the same in terms of diff size. Up to you.

clang/test/Modules/filename.cpp

2

So much nicer with split-file, thanks for updating!

clang/include/clang/Lex/HeaderSearch.h

public:
   ///
   /// \param IsFrameworkFound If non-null, will be set to true if a framework is
   /// found in any of searched SearchDirs. Will be set to false if a framework
   /// is found only through header maps. Doesn't guarantee the requested file is
   /// found.
   OptionalFileEntryRef LookupFile(
       StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
       ConstSearchDirIterator FromDir, ConstSearchDirIterator *CurDir,
-      ArrayRef<std::pair<const FileEntry *, const DirectoryEntry *>> Includers,
+      ArrayRef<std::pair<const FileEntry *, DirectoryEntryRef>> Includers,
       SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath,
       Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
       bool *IsMapped, bool *IsFrameworkFound, bool SkipCache = false,
       bool BuildSystemModule = false, bool OpenFile = true,
       bool CacheFailures = true);
 
   /// Look up a subframework for the specified \#include file.
   ///

clang/lib/Frontend/FrontendAction.cpp

if (CI.getLangOpts().CPlusPlusModules && Input.getKind().isHeaderUnit() &&
       !Input.getKind().isPreprocessed()) {
     StringRef FileName = Input.getFile();
     InputKind Kind = Input.getKind();
     if (Kind.getHeaderUnitKind() != InputKind::HeaderUnit_Abs) {
       assert(CI.hasPreprocessor() &&
              "trying to build a header unit without a Pre-processor?");
       HeaderSearch &HS = CI.getPreprocessor().getHeaderSearchInfo();
       // Relative searches begin from CWD.
-      const DirectoryEntry *Dir = nullptr;
-      if (auto DirOrErr = CI.getFileManager().getDirectory("."))
-        Dir = *DirOrErr;
-      SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 1> CWD;
-      CWD.push_back({nullptr, Dir});
+      auto Dir = CI.getFileManager().getOptionalDirectoryRef(".");
+      SmallVector<std::pair<const FileEntry *, DirectoryEntryRef>, 1> CWD;
+      CWD.push_back({nullptr, *Dir});
       OptionalFileEntryRef FE =
           HS.LookupFile(FileName, SourceLocation(),
                         /*Angled*/ Input.getKind().getHeaderUnitKind() ==
                             InputKind::HeaderUnit_System,
                         nullptr, nullptr, CWD, nullptr, nullptr, nullptr,
                         nullptr, nullptr, nullptr);
       if (!FE) {
         CI.getDiagnostics().Report(diag::err_module_header_file_not_found)

clang/lib/Lex/HeaderSearch.cpp

 /// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file,
 /// return null on failure.  isAngled indicates whether the file reference is
 /// for system \#include's or not (i.e. using <> instead of ""). Includers, if
 /// non-empty, indicates where the \#including file(s) are, in case a relative
 /// search is needed. Microsoft mode will pass all \#including files.
 OptionalFileEntryRef HeaderSearch::LookupFile(
     StringRef Filename, SourceLocation IncludeLoc, bool isAngled,
     ConstSearchDirIterator FromDir, ConstSearchDirIterator *CurDirArg,
-    ArrayRef<std::pair<const FileEntry *, const DirectoryEntry *>> Includers,
+    ArrayRef<std::pair<const FileEntry *, DirectoryEntryRef>> Includers,
     SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath,
     Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule,
     bool *IsMapped, bool *IsFrameworkFound, bool SkipCache,
     bool BuildSystemModule, bool OpenFile, bool CacheFailures) {
   ConstSearchDirIterator CurDirLocal = nullptr;
   ConstSearchDirIterator &CurDir = CurDirArg ? *CurDirArg : CurDirLocal;
 
   if (IsMapped)
OptionalFileEntryRef HeaderSearch::LookupFile(
   if (!Includers.empty() && !isAngled && !NoCurDirSearch) {
     SmallString<1024> TmpDir;
     bool First = true;
     for (const auto &IncluderAndDir : Includers) {
       const FileEntry *Includer = IncluderAndDir.first;
 
       // Concatenate the requested file onto the directory.
       // FIXME: Portability.  Filename concatenation should be in sys::Path.
-      TmpDir = IncluderAndDir.second->getName();
+      TmpDir = IncluderAndDir.second.getName();
       TmpDir.push_back('/');
       TmpDir.append(Filename.begin(), Filename.end());
 
       // FIXME: We don't cache the result of getFileInfo across the call to
       // getFileAndSuggestModule, because it's a reference to an element of
       // a container that could be reallocated across this call.
       //
       // If we have no includer, that means we're processing a #include
for (const auto &IncluderAndDir : Includers) {
         StringRef Framework = FromHFI.Framework;
 
         HeaderFileInfo &ToHFI = getFileInfo(&FE->getFileEntry());
         ToHFI.DirInfo = DirInfo;
         ToHFI.IndexHeaderMapHeader = IndexHeaderMapHeader;
         ToHFI.Framework = Framework;
 
         if (SearchPath) {
-          StringRef SearchPathRef(IncluderAndDir.second->getName());
+          StringRef SearchPathRef(IncluderAndDir.second.getName());
           SearchPath->clear();
           SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
         }
         if (RelativePath) {
           RelativePath->clear();
           RelativePath->append(Filename.begin(), Filename.end());
         }
         if (First) {
           diagnoseFrameworkInclude(Diags, IncludeLoc,
-                                   IncluderAndDir.second->getName(), Filename,
+                                   IncluderAndDir.second.getName(), Filename,
                                    &FE->getFileEntry());
           return FE;
         }
 
         // Otherwise, we found the path via MSVC header search rules.  If
         // -Wmsvc-include is enabled, we have to keep searching to see if we
         // would've found this header in -I or -isystem directories.
         if (Diags.isIgnored(diag::ext_pp_include_search_ms, IncludeLoc)) {
if (checkMSVCHeaderSearch(Diags, MSFE ? &MSFE->getFileEntry() : nullptr,
       if (SuggestedModule)
         *SuggestedModule = MSSuggestedModule;
       return MSFE;
     }
 
     bool FoundByHeaderMap = !IsMapped ? false : *IsMapped;
     if (!Includers.empty())
       diagnoseFrameworkInclude(
-          Diags, IncludeLoc, Includers.front().second->getName(), Filename,
+          Diags, IncludeLoc, Includers.front().second.getName(), Filename,
           &File->getFileEntry(), isAngled, FoundByHeaderMap);
 
     // Remember this location for the next lookup we do.
     cacheLookupSuccess(CacheLookup, It, IncludeLoc);
     return File;
   }
 
   // If we are including a file with a quoted include "foo.h" from inside

clang/lib/Lex/PPDirectives.cpp

OptionalFileEntryRef Preprocessor::LookupFile(
   ConstSearchDirIterator &CurDir = CurDirArg ? *CurDirArg : CurDirLocal;
 
   Module *RequestingModule = getModuleForLocation(
       FilenameLoc, LangOpts.ModulesValidateTextualHeaderIncludes);
   bool RequestingModuleIsModuleInterface = !SourceMgr.isInMainFile(FilenameLoc);
 
   // If the header lookup mechanism may be relative to the current inclusion
   // stack, record the parent #includes.
-  SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 16>
-      Includers;
+  SmallVector<std::pair<const FileEntry *, DirectoryEntryRef>, 16> Includers;
   bool BuildSystemModule = false;
   if (!FromDir && !FromFile) {
     FileID FID = getCurrentFileLexer()->getFileID();
-    const FileEntry *FileEnt = SourceMgr.getFileEntryForID(FID);
+    OptionalFileEntryRef FileEnt = SourceMgr.getFileEntryRefForID(FID);
 
     // If there is no file entry associated with this file, it must be the
     // predefines buffer or the module includes buffer. Any other file is not
     // lexed with a normal lexer, so it won't be scanned for preprocessor
     // directives.
     //
     // If we have the predefines buffer, resolve #include references (which come
     // from the -include command line argument) from the current working
     // directory instead of relative to the main file.
     //
     // If we have the module includes buffer, resolve #include references (which
     // come from header declarations in the module map) relative to the module
     // map file.
     if (!FileEnt) {
       if (FID == SourceMgr.getMainFileID() && MainFileDir) {
         Includers.push_back(std::make_pair(nullptr, *MainFileDir));
         BuildSystemModule = getCurrentModule()->IsSystem;
-      } else if ((FileEnt =
-                    SourceMgr.getFileEntryForID(SourceMgr.getMainFileID())))
-        Includers.push_back(std::make_pair(FileEnt, *FileMgr.getDirectory(".")));
+      } else if ((FileEnt = SourceMgr.getFileEntryRefForID(
+                      SourceMgr.getMainFileID()))) {
+        auto CWD = FileMgr.getOptionalDirectoryRef(".");
+        Includers.push_back(std::make_pair(*FileEnt, *CWD));
+      }
     } else {
-      Includers.push_back(std::make_pair(FileEnt, FileEnt->getDir()));
+      Includers.push_back(std::make_pair(*FileEnt, FileEnt->getDir()));
     }
 
     // MSVC searches the current include stack from top to bottom for
     // headers included by quoted include directives.
     // See: http://msdn.microsoft.com/en-us/library/36k2cdd4.aspx
     if (LangOpts.MSVCCompat && !isAngled) {
       for (IncludeStackInfo &ISEntry : llvm::reverse(IncludeMacroStack)) {
         if (IsFileLexer(ISEntry))
           if ((FileEnt = ISEntry.ThePPLexer->getFileEntry()))
-            Includers.push_back(std::make_pair(FileEnt, FileEnt->getDir()));
+            Includers.push_back(std::make_pair(*FileEnt, FileEnt->getDir()));
       }
     }
   }
 
   CurDir = CurDirLookup;
 
   if (FromFile) {
     // We're supposed to start looking from after a particular file. Search

clang/test/Modules/Inputs/filename/a.h

-const char *p = __FILE__;

clang/test/Modules/Inputs/filename/module.map

-module "A" {
-  header "a.h"
-}

clang/test/Modules/filename.cpp

-// RUN: cd %S
-// RUN: %clang_cc1 -I. -fmodule-name=A  -fmodule-map-file=%S/Inputs/filename/module.map %s -E | FileCheck %s
+// RUN: rm -rf %t
+// RUN: split-file %s %t
 
-#include "Inputs/filename/a.h"
+//--- include/a.h
+const char *p = __FILE__;
+//--- include/module.modulemap
+module "A" { header "a.h" }
+//--- src/tu.cpp
+#include "a.h"
+
+// RUN: cd %t
+// RUN: %clang_cc1 -I ./include -fmodule-name=A -fmodule-map-file=%t/include/module.modulemap %t/src/tu.cpp -E | FileCheck %s
 
 // Make sure that headers that are referenced by module maps have __FILE__
-// reflect the include path they were found with.
-// CHECK: const char *p = "./Inputs/filename/a.h"
+// reflect the include path they were found with. (We make sure they cannot be
+// found relative to the includer.)
+// CHECK: const char *p = "./include{{/|\\\\}}a.h"