[Lex] Make HeaderMaps a unique_ptr vector

[MaskRay]

Summary

unique_ptr makes the ownership clearer than a raw pointer container.

[kristina]

Given the context (class an file name itself) and documentation around the function, I don't think in this particular case it improves readability or maintainability, the lifetime of the HeaderMap is (IMHO) fairly obvious from the const qualifier and from the documentation of the function itself. I would say leave it as is.

[dblaikie]

In D50945#1205337, @kristina wrote:

Given the context (class an file name itself) and documentation around the function, I don't think in this particular case it improves readability or maintainability, the lifetime of the HeaderMap is (IMHO) fairly obvious from the const qualifier and from the documentation of the function itself. I would say leave it as is.

I'd vote in favor of the change - since it makes the ownership obvious at-a-glance (documentation or not) & helps catch any mistakes that can be easily introduced when handling object ownership.

[dblaikie]

Looks good - thanks!

[MaskRay]

Thanks!

include/clang/Lex/HeaderMap.h

 /// to create and process than a directory of symlinks.
 class HeaderMap : private HeaderMapImpl {
   HeaderMap(std::unique_ptr<const llvm::MemoryBuffer> File, bool BSwap)
       : HeaderMapImpl(std::move(File), BSwap) {}
 
 public:
   /// This attempts to load the specified file as a header map.  If it doesn't
   /// look like a HeaderMap, it gives up and returns null.
-  static const HeaderMap *Create(const FileEntry *FE, FileManager &FM);
+  static std::unique_ptr<HeaderMap> Create(const FileEntry *FE,
+                                           FileManager &FM);
 
   /// Check to see if the specified relative filename is located in this
   /// HeaderMap.  If so, open it and return its FileEntry.  If RawPath is not
   /// NULL and the file is found, RawPath will be set to the raw path at which
   /// the file was found in the file system. For example, for a search path
   /// ".." and a filename "../file.h" this would be "../../file.h".
   const FileEntry *LookupFile(StringRef Filename, FileManager &FM) const;
 
   using HeaderMapImpl::lookupFilename;
   using HeaderMapImpl::getFileName;
   using HeaderMapImpl::dump;
 };
 
 } // end namespace clang.
 
 #endif

include/clang/Lex/HeaderSearch.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_LEX_HEADERSEARCH_H
 #define LLVM_CLANG_LEX_HEADERSEARCH_H
 
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/DirectoryLookup.h"
+#include "clang/Lex/HeaderMap.h"
 #include "clang/Lex/ModuleMap.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Allocator.h"
 #include <cassert>
 #include <cstddef>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {
 
 class DiagnosticsEngine;
 class DirectoryEntry;
 class ExternalPreprocessorSource;
 class FileEntry;
 class FileManager;
-class HeaderMap;
 class HeaderSearchOptions;
 class IdentifierInfo;
 class LangOptions;
 class Module;
 class Preprocessor;
 class TargetInfo;
 
 /// The preprocessor keeps track of this information for each
class HeaderSearch {
 
   /// Maps include file names (including the quotes or
   /// angle brackets) to other include file names.  This is used to support the
   /// include_alias pragma for Microsoft compatibility.
   using IncludeAliasMap =
       llvm::StringMap<std::string, llvm::BumpPtrAllocator>;
   std::unique_ptr<IncludeAliasMap> IncludeAliases;
 
-  /// This is a mapping from FileEntry -> HeaderMap, uniquing
-  /// headermaps.  This vector owns the headermap.
-  std::vector<std::pair<const FileEntry *, const HeaderMap *>> HeaderMaps;
+  /// This is a mapping from FileEntry -> HeaderMap, uniquing headermaps.
+  std::vector<std::pair<const FileEntry *, std::unique_ptr<HeaderMap>>> HeaderMaps;
 
   /// The mapping between modules and headers.
   mutable ModuleMap ModMap;
 
   /// Describes whether a given directory has a module map in it.
   llvm::DenseMap<const DirectoryEntry *, bool> DirectoryHasModuleMap;
 
   /// Set of module map files we've already loaded, and a flag indicating
class HeaderSearch {
   unsigned NumSubFrameworkLookups = 0;
 
 public:
   HeaderSearch(std::shared_ptr<HeaderSearchOptions> HSOpts,
                SourceManager &SourceMgr, DiagnosticsEngine &Diags,
                const LangOptions &LangOpts, const TargetInfo *Target);
   HeaderSearch(const HeaderSearch &) = delete;
   HeaderSearch &operator=(const HeaderSearch &) = delete;
-  ~HeaderSearch();
 
   /// Retrieve the header-search options with which this header search
   /// was initialized.
   HeaderSearchOptions &getHeaderSearchOpts() const { return *HSOpts; }
 
   FileManager &getFileMgr() const { return FileMgr; }
 
   DiagnosticsEngine &getDiags() const { return Diags; }

lib/Lex/HeaderMap.cpp

 //===----------------------------------------------------------------------===//
 // Verification and Construction
 //===----------------------------------------------------------------------===//
 
 /// HeaderMap::Create - This attempts to load the specified file as a header
 /// map.  If it doesn't look like a HeaderMap, it gives up and returns null.
 /// If it looks like a HeaderMap but is obviously corrupted, it puts a reason
 /// into the string error argument and returns null.
-const HeaderMap *HeaderMap::Create(const FileEntry *FE, FileManager &FM) {
+std::unique_ptr<HeaderMap> HeaderMap::Create(const FileEntry *FE,
+                                             FileManager &FM) {
   // If the file is too small to be a header map, ignore it.
   unsigned FileSize = FE->getSize();
   if (FileSize <= sizeof(HMapHeader)) return nullptr;
 
   auto FileBuffer = FM.getBufferForFile(FE);
   if (!FileBuffer || !*FileBuffer)
     return nullptr;
   bool NeedsByteSwap;
   if (!checkHeader(**FileBuffer, NeedsByteSwap))
     return nullptr;
-  return new HeaderMap(std::move(*FileBuffer), NeedsByteSwap);
+  return std::unique_ptr<HeaderMap>(new HeaderMap(std::move(*FileBuffer), NeedsByteSwap));
 }
 
 bool HeaderMapImpl::checkHeader(const llvm::MemoryBuffer &File,
                                 bool &NeedsByteSwap) {
   if (File.getBufferSize() <= sizeof(HMapHeader))
     return false;
   const char *FileStart = File.getBufferStart();
 

lib/Lex/HeaderSearch.cpp

 HeaderSearch::HeaderSearch(std::shared_ptr<HeaderSearchOptions> HSOpts,
                            SourceManager &SourceMgr, DiagnosticsEngine &Diags,
                            const LangOptions &LangOpts,
                            const TargetInfo *Target)
     : HSOpts(std::move(HSOpts)), Diags(Diags),
       FileMgr(SourceMgr.getFileManager()), FrameworkMap(64),
       ModMap(SourceMgr, Diags, LangOpts, Target, *this) {}
 
-HeaderSearch::~HeaderSearch() {
-  // Delete headermaps.
-  for (unsigned i = 0, e = HeaderMaps.size(); i != e; ++i)
-    delete HeaderMaps[i].second;
-}
-
 void HeaderSearch::PrintStats() {
   fprintf(stderr, "\n*** HeaderSearch Stats:\n");
   fprintf(stderr, "%d files tracked.\n", (int)FileInfo.size());
   unsigned NumOnceOnlyFiles = 0, MaxNumIncludes = 0, NumSingleIncludedFiles = 0;
   for (unsigned i = 0, e = FileInfo.size(); i != e; ++i) {
     NumOnceOnlyFiles += FileInfo[i].isImport;
     if (MaxNumIncludes < FileInfo[i].NumIncludes)
       MaxNumIncludes = FileInfo[i].NumIncludes;

 const HeaderMap *HeaderSearch::CreateHeaderMap(const FileEntry *FE) {
   // We expect the number of headermaps to be small, and almost always empty.
   // If it ever grows, use of a linear search should be re-evaluated.
   if (!HeaderMaps.empty()) {
     for (unsigned i = 0, e = HeaderMaps.size(); i != e; ++i)
       // Pointer equality comparison of FileEntries works because they are
       // already uniqued by inode.
       if (HeaderMaps[i].first == FE)
-        return HeaderMaps[i].second;
+        return HeaderMaps[i].second.get();
   }
 
-  if (const HeaderMap *HM = HeaderMap::Create(FE, FileMgr)) {
-    HeaderMaps.push_back(std::make_pair(FE, HM));
-    return HM;
+  if (std::unique_ptr<HeaderMap> HM = HeaderMap::Create(FE, FileMgr)) {
+    HeaderMaps.emplace_back(FE, std::move(HM));
+    return HeaderMaps.back().second.get();
   }
 
   return nullptr;
 }
 
 /// Get filenames for all registered header maps.
 void HeaderSearch::getHeaderMapFileNames(
     SmallVectorImpl<std::string> &Names) const {