[PATCH] D59053: [LLD][COFF] Early dependency detection

[Alexandre Ganea via Phabricator via llvm-commits]

aganea added inline comments.


================
Comment at: COFF/DebugTypes.cpp:46
+
+static std::vector<std::unique_ptr<TpiSource>> GC;
+
----------------
ruiu wrote:
> What is this? You are not using this. (If you need it in another patch, move this to the another patch.)
> 
Normally we should have `std::unique_ptr<TpiSource> DebugTypesObj` in `COFF/InputFiles.h, L166`, but I wanted to avoid #including DebugTypes.h just so we can use `std::unique_ptr`. I'm keeping hard references to `TpiSource` objects here instead.


================
Comment at: COFF/InputFiles.cpp:688
+  if (IsPCH) {
+    DebugTypesObj = makeTpiSource(TpiSource::PCH, *this);
+    return;
----------------
ruiu wrote:
> If you do know the first argument for all function calls of `makeTpiSource`, you can define one function for each value of the first argument, instead of calling the same function and then dispatch according to the first argument.
I did like you said - however now there are details leaking out of `DebugTypes.h`, like `TypeServer2Record` parameter to `makeUseTypeServerSource()`, which now requires having `#include "llvm/DebugInfo/CodeView/TypeRecord.h"` directly in `DebugTypes.h`, which can slow down compilation - if `DebugTypes.h` is included elsewhere. It's a tradeoff between terseness and compilation speed, please let me know which you prefer.


================
Comment at: COFF/InputFiles.cpp:693
+  if (FirstType->kind() == LF_TYPESERVER2) {
+    auto TS = cantFail(
+        TypeDeserializer::deserializeAs<TypeServer2Record>(FirstType->data()));
----------------
ruiu wrote:
> Avoid `auto`.
The right side already specifies that it deserializes as a `TypeServer2Record`, but yes I made that explicit.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59053/new/

https://reviews.llvm.org/D59053