[PATCH] D45213: [COFF][LLD] Add link support for precompiled headers .objs

[Zachary Turner via Phabricator via llvm-commits]

zturner added a comment.

Sorry for the delay, been OOO for 3 days.

BTW, When you upload the next version, can you make sure it has full context?



================
Comment at: lld/COFF/PDB.cpp:258
+maybeReadRecord(CVTypeArray &Types, const uint32_t RecordTypeValue,
+                const bool drop = false) {
   auto I = Types.begin();
----------------
Variable names should be `CamelCase`.


================
Comment at: lld/COFF/PDB.cpp:293
   // merge any type information.
-  if (Optional<TypeServer2Record> TS = maybeReadTypeServerRecord(Types))
-    return maybeMergeTypeServerPDB(File, *TS);
+  if (Optional<TypeServer2Record> TS =
+                      maybeReadRecord<TypeServer2Record>(Types, LF_TYPESERVER2))
----------------
Would it be possible to write this as something like:

```
if (isExternalTypeStreamRecord(Types))
  return handleExternalTypeStreamRecord(Types);
```

Then this new handle function could handle both `LF_TYPESERVER2` and `LF_PRECOMP`, and we wouldn't need the weird function with the boolean anymore


================
Comment at: lld/COFF/PDB.cpp:500-501
+
+  if (CurrentPath.empty())
+  {
+    // If the precomp object was on a different path, we can still use it,
----------------
This doesn't look clang formatted.


================
Comment at: lld/COFF/PDB.cpp:506
+    CurrentPath.clear();
+    for (ObjFile *f : ObjFile::Instances) {
+      SmallString<128> path = f->getName();
----------------
Upper case `F`


================
Comment at: lld/COFF/PDB.cpp:507-510
+      SmallString<128> path = f->getName();
+      sys::fs::make_absolute(path);
+      sys::path::native(path, sys::path::Style::windows);
+      StringRef filename = sys::path::filename(path);
----------------
Same here.  Just fix all variable names actually.


================
Comment at: lld/COFF/PDB.cpp:542-544
+  static CVIndexMap emptyMap;
+  emptyMap.IsPrecompiledTypeMap = true;
+  return emptyMap;
----------------
Eww.  How about returning an `Expected<const CVIndexMap&>` instead?


================
Comment at: llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp:489-492
+  // For .obj files containing precompiled types, we need to extract the
+  // signature, through EndPrecompRecord. This is done here for performance
+  // reasons, to avoid re-parsing the Types stream; in most cases, this .obj
+  // will not be a precompiled types file.
----------------
I noticed that when precompiled headers are used, the `S_OBJNAME` symbol also has a matching signature.  Can / should this be used somehow?


Repository:
  rL LLVM

https://reviews.llvm.org/D45213