Code review feedback from Dave Ungar

Author: slavapestov

include/swift/AST/FineGrainedDependencyFormat.h

@@ -33,6 +33,8 @@ using llvm::BCVBR;
 using llvm::BCBlob;
 using llvm::BCRecordLayout;
 
+/// Every .swiftdeps file begins with these 4 bytes, for easy identification when
+/// debugging.
 const unsigned char FINE_GRAINED_DEPDENENCY_FORMAT_SIGNATURE[] = {'D', 'E', 'P', 'S'};
 
 const unsigned FINE_GRAINED_DEPENDENCY_FORMAT_VERSION_MAJOR = 1;
@@ -47,6 +49,13 @@ using DeclAspectField = BCFixed<1>;
 
 const unsigned RECORD_BLOCK_ID = llvm::bitc::FIRST_APPLICATION_BLOCKID;
 
+/// The swiftdeps file format consists of a METADATA record, followed by zero or more
+/// IDENTIFIER_NODE records.
+///
+/// Then, there is one SOURCE_FILE_DEP_GRAPH_NODE for each serialized
+/// SourceFileDepGraphNode. These are followed by FINGERPRINT_NODE and
+/// DEPENDS_ON_DEFINITION_NODE, if the node has a fingerprint and depends-on
+/// definitions, respectively.
 namespace record_block {
   enum {
     METADATA = 1,
@@ -56,47 +65,65 @@ namespace record_block {
     IDENTIFIER_NODE,
   };
 
+  // Always the first record in the file.
   using MetadataLayout = BCRecordLayout<
     METADATA, // ID
     BCFixed<16>, // Dependency graph format major version
     BCFixed<16>, // Dependency graph format minor version
     BCBlob // Compiler version string
   >;
 
+  // After the metadata record, we have zero or more identifier records,
+  // for each unique string that is referenced from a SourceFileDepGraphNode.
+  //
+  // Identifiers are referenced by their sequence number, starting from 1.
+  // The identifier value 0 is special; it always represents the empty string.
+  // There is no IDENTIFIER_NODE serialized that corresponds to it, instead
+  // the first IDENTIFIER_NODE always has a sequence number of 1.
+  using IdentifierNodeLayout = BCRecordLayout<
+    IDENTIFIER_NODE,
+    BCBlob
+  >;
+
   using SourceFileDepGraphNodeLayout = BCRecordLayout<
     SOURCE_FILE_DEP_GRAPH_NODE, // ID
-    NodeKindField, // Dependency key node kind
-    DeclAspectField, // Dependency key declaration aspect
-    IdentifierIDField, // Dependency key mangled context type name
-    IdentifierIDField, // Dependency key basic name
+    // The next four fields correspond to the fields of the DependencyKey
+    // structure.
+    NodeKindField, // DependencyKey::kind
+    DeclAspectField, // DependencyKey::aspect
+    IdentifierIDField, // DependencyKey::context
+    IdentifierIDField, // DependencyKey::name
     BCFixed<1> // Is this a "provides" node?
   >;
 
-  // Optionally follows DEPENDS_ON_DEFINITION_NODE.
+  // Follows DEPENDS_ON_DEFINITION_NODE when the SourceFileDepGraphNode has a
+  // fingerprint set.
   using FingerprintNodeLayout = BCRecordLayout<
     FINGERPRINT_NODE,
     BCBlob
   >;
 
-  // Optionally follows SOURCE_FILE_DEP_GRAPH_NODE and FINGERPRINT_NODE.
+  // Follows SOURCE_FILE_DEP_GRAPH_NODE and FINGERPRINT_NODE when the
+  // SourceFileDepGraphNode has one or more depends-on entries.
   using DependsOnDefNodeLayout = BCRecordLayout<
     DEPENDS_ON_DEFINITION_NODE,
-    BCVBR<16>
-  >;
-
-  // Optionally follows all other nodes.
-  using IdentifierNodeLayout = BCRecordLayout<
-    IDENTIFIER_NODE,
-    BCBlob
+    BCVBR<16> // The sequence number (starting from 0) of the referenced
+              // SOURCE_FILE_DEP_GRAPH_NODE
   >;
 }
 
+/// Tries to read the dependency graph from the given buffer.
+/// Returns true if there was an error.
 bool readFineGrainedDependencyGraph(llvm::MemoryBuffer &buffer,
                                     SourceFileDepGraph &g);
 
+/// Tries to read the dependency graph from the given path name.
+/// Returns true if there was an error.
 bool readFineGrainedDependencyGraph(llvm::StringRef path,
                                     SourceFileDepGraph &g);
 
+/// Tries to write the dependency graph to the given path name.
+/// Returns true if there was an error.
 bool writeFineGrainedDependencyGraph(DiagnosticEngine &diags, llvm::StringRef path,
                                      const SourceFileDepGraph &g);
 

lib/AST/FineGrainedDependencies.cpp

@@ -52,21 +52,11 @@ Optional<SourceFileDepGraph> SourceFileDepGraph::loadFromPath(StringRef path) {
 
 Optional<SourceFileDepGraph>
 SourceFileDepGraph::loadFromBuffer(llvm::MemoryBuffer &buffer) {
-  if (false) {
-    SourceFileDepGraph fg;
-    llvm::yaml::Input yamlReader(llvm::MemoryBufferRef(buffer), nullptr);
-    yamlReader >> fg;
-    if (yamlReader.error())
-      return None;
-    // return fg; compiles for Mac but not Linux, because it cannot be copied.
-    return Optional<SourceFileDepGraph>(std::move(fg));
-  } else {
-    SourceFileDepGraph fg;
-    if (swift::fine_grained_dependencies::readFineGrainedDependencyGraph(
-        buffer, fg))
-      return None;
-    return Optional<SourceFileDepGraph>(std::move(fg));
-  }
+  SourceFileDepGraph fg;
+  if (swift::fine_grained_dependencies::readFineGrainedDependencyGraph(
+      buffer, fg))
+    return None;
+  return Optional<SourceFileDepGraph>(std::move(fg));
 }
 
 //==============================================================================

lib/AST/FineGrainedDependencyFormat.cpp

@@ -35,6 +35,7 @@ class Deserializer {
   SmallVector<uint64_t, 64> Scratch;
   StringRef BlobData;
 
+  // These all return true if there was an error.
   bool readSignature();
   bool enterTopLevelBlock();
   bool readMetadata();
@@ -63,6 +64,8 @@ bool Deserializer::readSignature() {
 }
 
 bool Deserializer::enterTopLevelBlock() {
+  // Read the BLOCKINFO_BLOCK, which contains metadata used when dumping
+  // the binary data with llvm-bcanalyzer.
   {
     auto next = Cursor.advance();
     if (!next) {
@@ -80,6 +83,7 @@ bool Deserializer::enterTopLevelBlock() {
       return true;
   }
 
+  // Enters our subblock, which contains the actual dependency information.
   {
     auto next = Cursor.advance();
     if (!next) {

lib/AST/FrontendSourceFileDepGraphFactory.cpp

@@ -292,18 +292,7 @@ bool fine_grained_dependencies::emitReferenceDependencies(
                              SF, outputPath, depTracker, alsoEmitDotFile)
                               .construct();
 
-  bool hadError = false;
-  if (false) {
-    hadError =
-      withOutputFile(diags, outputPath, [&](llvm::raw_pwrite_stream &out) {
-        out << g.yamlProlog(SF->getASTContext().hadError());
-        llvm::yaml::Output yamlWriter(out);
-        yamlWriter << g;
-        return false;
-      });
-  } else {
-    hadError = writeFineGrainedDependencyGraph(diags, outputPath, g);
-  }
+  bool hadError = writeFineGrainedDependencyGraph(diags, outputPath, g);
 
   // If path is stdout, cannot read it back, so check for "-"
   assert(outputPath == "-" || g.verifyReadsWhatIsWritten(outputPath));