[APINotes] Upstream APINotesManager #72389

egorzhdan · 2023-11-15T13:50:27Z

This upstreams more of the Clang API Notes functionality that is currently implemented in the Apple fork: https://github.com/apple/llvm-project/tree/next/clang/lib/APINotes

llvmbot · 2023-11-15T13:50:56Z

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Egor Zhdan (egorzhdan)

Changes

This upstreams more of the Clang API Notes functionality that is currently implemented in the Apple fork: https://github.com/apple/llvm-project/tree/next/clang/lib/APINotes

Patch is 35.64 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/72389.diff

10 Files Affected:

(added) clang/include/clang/APINotes/APINotesManager.h (+163)
(modified) clang/include/clang/APINotes/Types.h (+3)
(modified) clang/include/clang/Basic/DiagnosticCommonKinds.td (+13)
(modified) clang/include/clang/Basic/LangOptions.def (+2)
(modified) clang/include/clang/Basic/Module.h (+3)
(added) clang/include/clang/Basic/SourceMgrAdapter.h (+85)
(added) clang/lib/APINotes/APINotesManager.cpp (+469)
(modified) clang/lib/APINotes/CMakeLists.txt (+1)
(modified) clang/lib/Basic/CMakeLists.txt (+1)
(added) clang/lib/Basic/SourceMgrAdapter.cpp (+136)

diff --git a/clang/include/clang/APINotes/APINotesManager.h b/clang/include/clang/APINotes/APINotesManager.h
new file mode 100644
index 000000000000000..d82d1fa2d544a1d
--- /dev/null
+++ b/clang/include/clang/APINotes/APINotesManager.h
@@ -0,0 +1,163 @@
+//===--- APINotesManager.h - Manage API Notes Files -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_APINOTES_APINOTESMANAGER_H
+#define LLVM_CLANG_APINOTES_APINOTESMANAGER_H
+
+#include "clang/Basic/Module.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/PointerUnion.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/VersionTuple.h"
+#include <memory>
+#include <string>
+
+namespace clang {
+
+class DirectoryEntry;
+class FileEntry;
+class LangOptions;
+class SourceManager;
+
+namespace api_notes {
+
+class APINotesReader;
+
+/// The API notes manager helps find API notes associated with declarations.
+///
+/// API notes are externally-provided annotations for declarations that can
+/// introduce new attributes (covering availability, nullability of
+/// parameters/results, and so on) for specific declarations without directly
+/// modifying the headers that contain those declarations.
+///
+/// The API notes manager is responsible for finding and loading the
+/// external API notes files that correspond to a given header. Its primary
+/// operation is \c findAPINotes(), which finds the API notes reader that
+/// provides information about the declarations at that location.
+class APINotesManager {
+  using ReaderEntry = llvm::PointerUnion<DirectoryEntryRef, APINotesReader *>;
+
+  SourceManager &SM;
+
+  /// Whether to implicitly search for API notes files based on the
+  /// source file from which an entity was declared.
+  bool ImplicitAPINotes;
+
+  /// The Swift version to use when interpreting versioned API notes.
+  llvm::VersionTuple SwiftVersion;
+
+  /// API notes readers for the current module.
+  ///
+  /// There can be up to two of these, one for public headers and one
+  /// for private headers.
+  APINotesReader *CurrentModuleReaders[2] = {nullptr, nullptr};
+
+  /// A mapping from header file directories to the API notes reader for
+  /// that directory, or a redirection to another directory entry that may
+  /// have more information, or NULL to indicate that there is no API notes
+  /// reader for this directory.
+  llvm::DenseMap<const DirectoryEntry *, ReaderEntry> Readers;
+
+  /// Load the API notes associated with the given file, whether it is
+  /// the binary or source form of API notes.
+  ///
+  /// \returns the API notes reader for this file, or null if there is
+  /// a failure.
+  std::unique_ptr<APINotesReader> loadAPINotes(FileEntryRef APINotesFile);
+
+  /// Load the API notes associated with the given buffer, whether it is
+  /// the binary or source form of API notes.
+  ///
+  /// \returns the API notes reader for this file, or null if there is
+  /// a failure.
+  std::unique_ptr<APINotesReader> loadAPINotes(StringRef Buffer);
+
+  /// Load the given API notes file for the given header directory.
+  ///
+  /// \param HeaderDir The directory at which we
+  ///
+  /// \returns true if an error occurred.
+  bool loadAPINotes(const DirectoryEntry *HeaderDir, FileEntryRef APINotesFile);
+
+  /// Look for API notes in the given directory.
+  ///
+  /// This might find either a binary or source API notes.
+  OptionalFileEntryRef findAPINotesFile(DirectoryEntryRef Directory,
+                                        StringRef FileName,
+                                        bool WantPublic = true);
+
+  /// Attempt to load API notes for the given framework.
+  ///
+  /// \param FrameworkPath The path to the framework.
+  /// \param Public Whether to load the public API notes. Otherwise, attempt
+  /// to load the private API notes.
+  ///
+  /// \returns the header directory entry (e.g., for Headers or PrivateHeaders)
+  /// for which the API notes were successfully loaded, or NULL if API notes
+  /// could not be loaded for any reason.
+  OptionalDirectoryEntryRef loadFrameworkAPINotes(llvm::StringRef FrameworkPath,
+                                                  llvm::StringRef FrameworkName,
+                                                  bool Public);
+
+public:
+  APINotesManager(SourceManager &SM, const LangOptions &LangOpts);
+  ~APINotesManager();
+
+  /// Set the Swift version to use when filtering API notes.
+  void setSwiftVersion(llvm::VersionTuple Version) {
+    this->SwiftVersion = Version;
+  }
+
+  /// Load the API notes for the current module.
+  ///
+  /// \param M The current module.
+  /// \param LookInModule Whether to look inside the module itself.
+  /// \param SearchPaths The paths in which we should search for API notes
+  /// for the current module.
+  ///
+  /// \returns true if API notes were successfully loaded, \c false otherwise.
+  bool loadCurrentModuleAPINotes(Module *M, bool LookInModule,
+                                 ArrayRef<std::string> SearchPaths);
+
+  /// Get FileEntry for the APINotes of the current module.
+  ///
+  /// \param M The current module.
+  /// \param LookInModule Whether to look inside the module itself.
+  /// \param SearchPaths The paths in which we should search for API notes
+  /// for the current module.
+  ///
+  /// \returns a vector of FileEntry where APINotes files are.
+  llvm::SmallVector<FileEntryRef, 2>
+  getCurrentModuleAPINotes(Module *M, bool LookInModule,
+                           ArrayRef<std::string> SearchPaths);
+
+  /// Load Compiled API notes for current module.
+  ///
+  /// \param Buffers Array of compiled API notes.
+  ///
+  /// \returns true if API notes were successfully loaded, \c false otherwise.
+  bool loadCurrentModuleAPINotesFromBuffer(ArrayRef<StringRef> Buffers);
+
+  /// Retrieve the set of API notes readers for the current module.
+  ArrayRef<APINotesReader *> getCurrentModuleReaders() const {
+    unsigned numReaders =
+        static_cast<unsigned>(CurrentModuleReaders[0] != nullptr) +
+        static_cast<unsigned>(CurrentModuleReaders[1] != nullptr);
+    return ArrayRef(CurrentModuleReaders).slice(0, numReaders);
+  }
+
+  /// Find the API notes readers that correspond to the given source location.
+  llvm::SmallVector<APINotesReader *, 2> findAPINotes(SourceLocation Loc);
+};
+
+} // end namespace api_notes
+} // end namespace clang
+
+#endif
diff --git a/clang/include/clang/APINotes/Types.h b/clang/include/clang/APINotes/Types.h
index 79595abcf7d02d9..d9c88adbf768e37 100644
--- a/clang/include/clang/APINotes/Types.h
+++ b/clang/include/clang/APINotes/Types.h
@@ -737,6 +737,9 @@ inline bool operator!=(const TypedefInfo &LHS, const TypedefInfo &RHS) {
   return !(LHS == RHS);
 }
 
+/// The file extension used for the source representation of API notes.
+static const char SOURCE_APINOTES_EXTENSION[] = "apinotes";
+
 /// Opaque context ID used to refer to an Objective-C class or protocol or a C++
 /// namespace.
 class ContextID {
diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 906ca6b8e47c50e..55a237f40af459f 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -390,6 +390,19 @@ def note_mt_message : Note<"[rewriter] %0">;
 def warn_arcmt_nsalloc_realloc : Warning<"[rewriter] call returns pointer to GC managed memory; it will become unmanaged in ARC">;
 def err_arcmt_nsinvocation_ownership : Error<"NSInvocation's %0 is not safe to be used with an object with ownership other than __unsafe_unretained">;
 
+// API notes
+def err_apinotes_message : Error<"%0">;
+def warn_apinotes_message : Warning<"%0">, InGroup<DiagGroup<"apinotes">>;
+def note_apinotes_message : Note<"%0">;
+
+class NonportablePrivateAPINotesPath  : Warning<
+  "private API notes file for module '%0' should be named "
+  "'%0_private.apinotes', not '%1'">;
+def warn_apinotes_private_case : NonportablePrivateAPINotesPath,
+  InGroup<DiagGroup<"nonportable-private-apinotes-path">>;
+def warn_apinotes_private_case_system : NonportablePrivateAPINotesPath, 
+  DefaultIgnore, InGroup<DiagGroup<"nonportable-private-system-apinotes-path">>;
+
 // C++ for OpenCL.
 def err_openclcxx_not_supported : Error<
   "'%0' is not supported in C++ for OpenCL">;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index c541ccefdd5fbe1..0f7e4783bd4f519 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -402,6 +402,8 @@ LANGOPT(XLPragmaPack, 1, 0, "IBM XL #pragma pack handling")
 
 LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
+LANGOPT(APINotes, 1, 0, "use external API notes")
+
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
                                            "field padding (0: none, 1:least "
                                            "aggressive, 2: more aggressive)")
diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h
index 08b153e8c1c9d33..d29cc0b45d583e0 100644
--- a/clang/include/clang/Basic/Module.h
+++ b/clang/include/clang/Basic/Module.h
@@ -178,6 +178,9 @@ class alignas(8) Module {
   /// eventually be exposed, for use in "private" modules.
   std::string ExportAsModule;
 
+  /// For the debug info, the path to this module's .apinotes file, if any.
+  std::string APINotesFile;
+
   /// Does this Module is a named module of a standard named module?
   bool isNamedModule() const {
     switch (Kind) {
diff --git a/clang/include/clang/Basic/SourceMgrAdapter.h b/clang/include/clang/Basic/SourceMgrAdapter.h
new file mode 100644
index 000000000000000..be7f9d5051fbf4c
--- /dev/null
+++ b/clang/include/clang/Basic/SourceMgrAdapter.h
@@ -0,0 +1,85 @@
+//=== SourceMgrAdapter.h - SourceMgr to SourceManager Adapter ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file provides an adapter that maps diagnostics from llvm::SourceMgr
+// to Clang's SourceManager.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_SOURCEMGRADAPTER_H
+#define LLVM_CLANG_SOURCEMGRADAPTER_H
+
+#include "clang/Basic/SourceManager.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/SourceMgr.h"
+#include <string>
+#include <utility>
+
+namespace clang {
+
+class DiagnosticsEngine;
+class FileEntry;
+
+/// An adapter that can be used to translate diagnostics from one or more
+/// llvm::SourceMgr instances to a ,
+class SourceMgrAdapter {
+  /// Clang source manager.
+  SourceManager &SrcMgr;
+
+  /// Clang diagnostics engine.
+  DiagnosticsEngine &Diagnostics;
+
+  /// Diagnostic IDs for errors, warnings, and notes.
+  unsigned ErrorDiagID, WarningDiagID, NoteDiagID;
+
+  /// The default file to use when mapping buffers.
+  OptionalFileEntryRef DefaultFile;
+
+  /// A mapping from (LLVM source manager, buffer ID) pairs to the
+  /// corresponding file ID within the Clang source manager.
+  llvm::DenseMap<std::pair<const llvm::SourceMgr *, unsigned>, FileID>
+      FileIDMapping;
+
+  /// Diagnostic handler.
+  static void handleDiag(const llvm::SMDiagnostic &Diag, void *Context);
+
+public:
+  /// Create a new \c SourceMgr adaptor that maps to the given source
+  /// manager and diagnostics engine.
+  SourceMgrAdapter(SourceManager &SM, DiagnosticsEngine &Diagnostics,
+                   unsigned ErrorDiagID, unsigned WarningDiagID,
+                   unsigned NoteDiagID,
+                   OptionalFileEntryRef DefaultFile = std::nullopt);
+
+  ~SourceMgrAdapter();
+
+  /// Map a source location in the given LLVM source manager to its
+  /// corresponding location in the Clang source manager.
+  SourceLocation mapLocation(const llvm::SourceMgr &LLVMSrcMgr,
+                             llvm::SMLoc Loc);
+
+  /// Map a source range in the given LLVM source manager to its corresponding
+  /// range in the Clang source manager.
+  SourceRange mapRange(const llvm::SourceMgr &LLVMSrcMgr, llvm::SMRange Range);
+
+  /// Handle the given diagnostic from an LLVM source manager.
+  void handleDiag(const llvm::SMDiagnostic &Diag);
+
+  /// Retrieve the diagnostic handler to use with the underlying SourceMgr.
+  llvm::SourceMgr::DiagHandlerTy getDiagHandler() {
+    return &SourceMgrAdapter::handleDiag;
+  }
+
+  /// Retrieve the context to use with the diagnostic handler produced by
+  /// \c getDiagHandler().
+  void *getDiagContext() { return this; }
+};
+
+} // end namespace clang
+
+#endif
diff --git a/clang/lib/APINotes/APINotesManager.cpp b/clang/lib/APINotes/APINotesManager.cpp
new file mode 100644
index 000000000000000..1e1ef66a3b78e59
--- /dev/null
+++ b/clang/lib/APINotes/APINotesManager.cpp
@@ -0,0 +1,469 @@
+//===--- APINotesManager.cpp - Manage API Notes Files ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/APINotes/APINotesManager.h"
+#include "clang/APINotes/APINotesReader.h"
+#include "clang/APINotes/APINotesYAMLCompiler.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/SourceMgrAdapter.h"
+#include "clang/Basic/Version.h"
+#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/PrettyStackTrace.h"
+
+using namespace clang;
+using namespace api_notes;
+
+#define DEBUG_TYPE "API Notes"
+STATISTIC(NumHeaderAPINotes, "non-framework API notes files loaded");
+STATISTIC(NumPublicFrameworkAPINotes, "framework public API notes loaded");
+STATISTIC(NumPrivateFrameworkAPINotes, "framework private API notes loaded");
+STATISTIC(NumFrameworksSearched, "frameworks searched");
+STATISTIC(NumDirectoriesSearched, "header directories searched");
+STATISTIC(NumDirectoryCacheHits, "directory cache hits");
+
+namespace {
+/// Prints two successive strings, which much be kept alive as long as the
+/// PrettyStackTrace entry.
+class PrettyStackTraceDoubleString : public llvm::PrettyStackTraceEntry {
+  StringRef First, Second;
+
+public:
+  PrettyStackTraceDoubleString(StringRef First, StringRef Second)
+      : First(First), Second(Second) {}
+  void print(raw_ostream &OS) const override { OS << First << Second; }
+};
+} // namespace
+
+APINotesManager::APINotesManager(SourceManager &SM, const LangOptions &LangOpts)
+    : SM(SM), ImplicitAPINotes(LangOpts.APINotes) {}
+
+APINotesManager::~APINotesManager() {
+  // Free the API notes readers.
+  for (const auto &Entry : Readers) {
+    if (auto Reader = Entry.second.dyn_cast<APINotesReader *>())
+      delete Reader;
+  }
+
+  delete CurrentModuleReaders[0];
+  delete CurrentModuleReaders[1];
+}
+
+std::unique_ptr<APINotesReader>
+APINotesManager::loadAPINotes(FileEntryRef APINotesFile) {
+  PrettyStackTraceDoubleString Trace("Loading API notes from ",
+                                     APINotesFile.getName());
+
+  // Open the source file.
+  auto SourceFileID = SM.getOrCreateFileID(APINotesFile, SrcMgr::C_User);
+  auto SourceBuffer = SM.getBufferOrNone(SourceFileID, SourceLocation());
+  if (!SourceBuffer)
+    return nullptr;
+
+  // Compile the API notes source into a buffer.
+  // FIXME: Either propagate OSType through or, better yet, improve the binary
+  // APINotes format to maintain complete availability information.
+  // FIXME: We don't even really need to go through the binary format at all;
+  // we're just going to immediately deserialize it again.
+  llvm::SmallVector<char, 1024> APINotesBuffer;
+  std::unique_ptr<llvm::MemoryBuffer> CompiledBuffer;
+  {
+    SourceMgrAdapter SMAdapter(
+        SM, SM.getDiagnostics(), diag::err_apinotes_message,
+        diag::warn_apinotes_message, diag::note_apinotes_message, APINotesFile);
+    llvm::raw_svector_ostream OS(APINotesBuffer);
+    if (api_notes::compileAPINotes(
+            SourceBuffer->getBuffer(), SM.getFileEntryForID(SourceFileID), OS,
+            SMAdapter.getDiagHandler(), SMAdapter.getDiagContext()))
+      return nullptr;
+
+    // Make a copy of the compiled form into the buffer.
+    CompiledBuffer = llvm::MemoryBuffer::getMemBufferCopy(
+        StringRef(APINotesBuffer.data(), APINotesBuffer.size()));
+  }
+
+  // Load the binary form we just compiled.
+  auto Reader = APINotesReader::Create(std::move(CompiledBuffer), SwiftVersion);
+  assert(Reader && "Could not load the API notes we just generated?");
+  return Reader;
+}
+
+std::unique_ptr<APINotesReader>
+APINotesManager::loadAPINotes(StringRef Buffer) {
+  llvm::SmallVector<char, 1024> APINotesBuffer;
+  std::unique_ptr<llvm::MemoryBuffer> CompiledBuffer;
+  SourceMgrAdapter SMAdapter(
+      SM, SM.getDiagnostics(), diag::err_apinotes_message,
+      diag::warn_apinotes_message, diag::note_apinotes_message, std::nullopt);
+  llvm::raw_svector_ostream OS(APINotesBuffer);
+
+  if (api_notes::compileAPINotes(Buffer, nullptr, OS,
+                                 SMAdapter.getDiagHandler(),
+                                 SMAdapter.getDiagContext()))
+    return nullptr;
+
+  CompiledBuffer = llvm::MemoryBuffer::getMemBufferCopy(
+      StringRef(APINotesBuffer.data(), APINotesBuffer.size()));
+  auto Reader = APINotesReader::Create(std::move(CompiledBuffer), SwiftVersion);
+  assert(Reader && "Could not load the API notes we just generated?");
+  return Reader;
+}
+
+bool APINotesManager::loadAPINotes(const DirectoryEntry *HeaderDir,
+                                   FileEntryRef APINotesFile) {
+  assert(Readers.find(HeaderDir) == Readers.end());
+  if (auto Reader = loadAPINotes(APINotesFile)) {
+    Readers[HeaderDir] = Reader.release();
+    return false;
+  }
+
+  Readers[HeaderDir] = nullptr;
+  return true;
+}
+
+OptionalFileEntryRef
+APINotesManager::findAPINotesFile(DirectoryEntryRef Directory,
+                                  StringRef Basename, bool WantPublic) {
+  FileManager &FM = SM.getFileManager();
+
+  llvm::SmallString<128> Path(Directory.getName());
+
+  StringRef BasenameSuffix = "";
+  if (!WantPublic)
+    BasenameSuffix = "_private";
+
+  // Look for the source API notes file.
+  llvm::sys::path::append(Path, llvm::Twine(Basename) + BasenameSuffix + "." +
+                                    SOURCE_APINOTES_EXTENSION);
+  return FM.getOptionalFileRef(Path, /*Open*/ true);
+}
+
+OptionalDirectoryEntryRef APINotesManager::loadFrameworkAPINotes(
+    llvm::StringRef FrameworkPath, llvm::StringRef FrameworkName, bool Public) {
+  FileManager &FM = SM.getFileManager();
+
+  llvm::SmallString<128> Path(FrameworkPath);
+  unsigned FrameworkNameLength = Path.size();
+
+  // Form the path to the APINotes file.
+  llvm::sys::path::append(Path, "APINotes");
+  if (Public)
+    llvm::sys::path::append(
+        Path, (llvm::Twine(FrameworkName) + "." + SOURCE_APINOTES_EXTENSION));
+  else
+    llvm::sys::path::append(Path, (llvm::Twine(FrameworkName) + "_private." +
+                                   SOURCE_APINOTES_EXTENSION));
+
+  // Try to open the APINotes file.
+  auto APINotesFile = FM.getOptionalFileRef(Path);
+  if (!APINotesFile)
+    return std::nullopt;
+
+  // Form the path to the corresponding header directory.
+  Path.resize(FrameworkNameLength);
+  if (Public)
+    llvm::sys::path::append(Path, "Headers");
+  else
+    llvm::sys::path...
[truncated]

clang/include/clang/APINotes/APINotesManager.h

compnerd · 2023-11-15T15:50:54Z

clang/include/clang/APINotes/APINotesManager.h

+  /// \returns the header directory entry (e.g., for Headers or PrivateHeaders)
+  /// for which the API notes were successfully loaded, or NULL if API notes
+  /// could not be loaded for any reason.
+  OptionalDirectoryEntryRef loadFrameworkAPINotes(llvm::StringRef FrameworkPath,


Why is the framework different from a library? Why not have the library also take a library path and a library name?

A framework will have the API notes file under either {framework root}/APINotes, {framework root}/Headers or {framework root}/PrivateHeaders, while a library will have API notes simply in its directory.

Can we add that as a comment? I think that makes sense but wasn't immediately obvious.

Good point, added a comment to reflect that

compnerd · 2023-11-15T15:51:36Z

clang/include/clang/APINotes/APINotesManager.h

+  /// Load the API notes for the current module.
+  ///
+  /// \param M The current module.
+  /// \param LookInModule Whether to look inside the module itself.


Can you clarify the "look inside the module itself"? It makes me think that it will be serialised into the PCM and that you are looking into that.

Clarified the comment to indicate that we really mean looking into the directory of the current module.

compnerd · 2023-11-15T15:53:58Z

clang/include/clang/APINotes/APINotesManager.h

+  ArrayRef<APINotesReader *> getCurrentModuleReaders() const {
+    unsigned numReaders =
+        static_cast<unsigned>(CurrentModuleReaders[0] != nullptr) +
+        static_cast<unsigned>(CurrentModuleReaders[1] != nullptr);


I would say lets split these out into booleans. Enumerate over them, and add an assert that private is never set when public is not set.

bool hasPublic = CurrentModuleReaders[Reader::Public]; bool hasPrivate = CurrentModuleReaders[Reader::Private]; assert(!hasPrivate || hasPublic && "private module requires public module"); return ArrayRef(CurrentModuleReaders).slice(0, hasPrivate ? 2 : 1);

Thanks, I changed your snippet a bit to handle !hasPrivate && !hasPublic

compnerd · 2023-11-15T15:59:14Z

clang/lib/APINotes/APINotesManager.cpp

+APINotesManager::~APINotesManager() {
+  // Free the API notes readers.
+  for (const auto &Entry : Readers) {
+    if (auto Reader = Entry.second.dyn_cast<APINotesReader *>())


Why is the dyn_cast needed here? The Readers is an array of APINotesReader *. Should we instead consider using std::unique_ptr<APINotesReader> Readers[2] to avoid this and allow C++ to manage the memory properly? Or are these readers allocated off a bump pointer allocator or something?

Not quite, Readers is a DenseMap where the value type is a PointerUnion, so we need to cast it to one of the possible types which is APINotesReader *.

I like the idea of making CurrentModuleReaders an array of unique pointers regardless. I'll go ahead and do that.

Just realized that using unique_ptrs here is a bit tricky, because we are storing these pointers in llvm::PointerUnion. Looks like we'd increase the memory footprint by abandoning llvm::PointerUnion. Do you think it's still worth refactoring this?

No, I think that we should keep the PointerUnion for the density. Perhaps add a little hint explaining that to the declaration?

Done, added a comment

clang/lib/APINotes/APINotesManager.cpp

github-actions · 2023-11-15T18:00:31Z

✅ With the latest revision this PR passed the C/C++ code formatter.

compnerd

Some minor comments left to address about comments, but the rest LGTM.

This upstreams more of the Clang API Notes functionality that is currently implemented in the Apple fork: https://github.com/apple/llvm-project/tree/next/clang/lib/APINotes

nico · 2023-11-17T16:56:07Z

I was wondering what this is. It looks like we have documentation that describes it at https://clang.llvm.org/docs/APINotes.html (in case anyone else is wondering too).

egorzhdan · 2023-11-20T13:38:31Z

@nico sorry for not leaving a more detailed description. I'll make sure to add a release note once the API notes are upstreamed entirely.

egorzhdan requested a review from compnerd November 15, 2023 13:50

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Nov 15, 2023

egorzhdan force-pushed the upstream-apinotes-manager branch from 2550d96 to fef3782 Compare November 15, 2023 15:54

compnerd reviewed Nov 15, 2023

View reviewed changes

egorzhdan force-pushed the upstream-apinotes-manager branch from fef3782 to aa80c53 Compare November 15, 2023 17:57

egorzhdan force-pushed the upstream-apinotes-manager branch from aa80c53 to 9e171fa Compare November 15, 2023 18:05

egorzhdan requested a review from compnerd November 15, 2023 18:06

compnerd approved these changes Nov 15, 2023

View reviewed changes

[APINotes] Upstream APINotesManager

c4c9e3b

This upstreams more of the Clang API Notes functionality that is currently implemented in the Apple fork: https://github.com/apple/llvm-project/tree/next/clang/lib/APINotes

egorzhdan force-pushed the upstream-apinotes-manager branch from 9e171fa to c4c9e3b Compare November 16, 2023 14:03

egorzhdan merged commit f049395 into llvm:main Nov 17, 2023
3 checks passed

egorzhdan deleted the upstream-apinotes-manager branch November 20, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APINotes] Upstream APINotesManager #72389

[APINotes] Upstream APINotesManager #72389

egorzhdan commented Nov 15, 2023

llvmbot commented Nov 15, 2023 •

edited

compnerd Nov 15, 2023

egorzhdan Nov 15, 2023

compnerd Nov 15, 2023

egorzhdan Nov 16, 2023

compnerd Nov 15, 2023

egorzhdan Nov 15, 2023

compnerd Nov 15, 2023

egorzhdan Nov 15, 2023

compnerd Nov 15, 2023

egorzhdan Nov 15, 2023

egorzhdan Nov 15, 2023

compnerd Nov 15, 2023

egorzhdan Nov 16, 2023

github-actions bot commented Nov 15, 2023 •

edited

compnerd left a comment

nico commented Nov 17, 2023

egorzhdan commented Nov 20, 2023

[APINotes] Upstream APINotesManager #72389

[APINotes] Upstream APINotesManager #72389

Conversation

egorzhdan commented Nov 15, 2023

llvmbot commented Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023 • edited

compnerd left a comment

Choose a reason for hiding this comment

nico commented Nov 17, 2023

egorzhdan commented Nov 20, 2023

llvmbot commented Nov 15, 2023 •

edited

github-actions bot commented Nov 15, 2023 •

edited