[PATCH] D55585: RFC: [LLD][COFF] Parallel GHASH generation at link-time

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Overall this is a huge patch, and I think it needs much more comments or a large file comment that gives the big picture what you are doing. I'm worried that that would not be able to understood once people who wrote or reviewed left the project.



================
Comment at: lld/trunk/COFF/Driver.cpp:608-613
+  if (ArgL == "md5")
+    Hasher = codeview::GloballyHashedType::HashType::MD5;
+  else if (ArgL == "sha1")
+    Hasher = codeview::GloballyHashedType::HashType::SHA1;
+  else if (ArgL == "cityhash")
+    Hasher = codeview::GloballyHashedType::HashType::CityHash;
----------------
What is the point of making it selectable to users? It feels to me that you should pick up the one that you think the best and just use it.


================
Comment at: lld/trunk/COFF/Driver.cpp:614
+    Hasher = codeview::GloballyHashedType::HashType::CityHash;
+  codeview::GloballyHashedType::H = Hasher;
+}
----------------
parse() functions should return a value instead of mutating a variable as a side effect.


================
Comment at: lld/trunk/COFF/PDB.cpp:96-97
 
+// A PDB type server, which might be a dependency of another OBJ
+class PDBDependency : public InputFile {
+public:
----------------
A dependency represented as an "input file" feels a odd concept. Only an object file and the like should inherit InputFile.


================
Comment at: lld/trunk/COFF/PDB.cpp:936
+    return computeTypeHashes(cast<PCHDependency>(File)->refObj(), Stream.Types);
+  } break;
+  case UsingPCH: {
----------------
Formatting.


================
Comment at: lld/trunk/Common/Summary.cpp:20-21
+using namespace llvm;
+
+static void printLine(Any Val, StringRef S) {
+  if (Val.isEqual(0))
----------------
Please don't use llvm::Any. Pass StringRef. If you need, define your own `lld::toString` to stringize an object you need to apss.


================
Comment at: lld/trunk/Common/Summary.cpp:26
+  llvm::raw_svector_ostream Stream(Str);
+  Stream << formatv("{0} {1}", fmt_align(Val, AlignStyle::Right, 15), S.data());
+
----------------
Please don't use formatv


================
Comment at: lld/trunk/Common/Summary.cpp:42
+  for (auto& L : Lines)
+  {
+    printLine(L.first, L.second);
----------------
Please use clang-format to format the entire patch.


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

https://reviews.llvm.org/D55585