[PATCH] D36758: [LLD COFF / PDB] Incrementally update the BuildId when writing a PDB.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/PDB.cpp:827
 
+  GUID Uuid{};
+  uint32_t Age = BuildId.PDB70.Age;
----------------
I believe you can remove `{}`.


================
Comment at: lld/COFF/PDB.cpp:831
   auto &InfoBuilder = Builder.getInfoBuilder();
-  InfoBuilder.setAge(DI ? DI->PDB70.Age : 0);
+  InfoBuilder.setAge(Age);
 
----------------
If `Age` is assigned only once, I'd remove that variable and use `BuildId.PDB70.Age` directly instead.


================
Comment at: lld/COFF/PDB.cpp:834
   GUID uuid{};
-  if (DI)
-    memcpy(&uuid, &DI->PDB70.Signature, sizeof(uuid));
+  memcpy(&uuid, &BuildId.PDB70.Signature, sizeof(uuid));
   InfoBuilder.setGuid(uuid);
----------------
Does this mean both `Uuid` and `uuid` are visible in this function? That is a bit confusing.


================
Comment at: lld/COFF/Writer.cpp:786
 
+void Writer::loadExistingBuildId(StringRef Path) {
+  // We don't need to incrementally update a previous build id if we're not
----------------
Looks like this function does not depend on `Writer` class. Can you make this a static non-member function that returns a `const codeview::DebugInfo *` (or a nullptr if no previous build id is available)?

Also I think this function needs comment as to why we want to keep existing build id if available.


https://reviews.llvm.org/D36758