[PATCH] D66825: [10/10] [LLD] [COFF] Silently drop a manifest with language 0, if there's another manifest with a nonzero language id

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

It looks better!



================
Comment at: llvm/lib/Object/WindowsResource.cpp:254
 
+// Remove default manifests (with language zero) if there are other manifests
+// present, and report an error if there are more than one manifest with a
----------------
Sorry to be a bit too picky, but I'd write more comments as to what the default manifest, why we may have duplicate default manifests, and why we can safely ignore them if there's any other manifest. I'd also explain that this is for mingw.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:282
+    return;
+  // More than one non-language-zero manifest
+  auto FirstIt = NameNode->IDChildren.begin();
----------------
This function doesn't have any blank line, so it looks too dense. I'd insert blank lines at the places where it makes sense.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:562
 
+void WindowsResourceParser::TreeNode::shiftDataIndexDown(uint32_t Index) {
+  if (IsDataNode && DataIndex >= Index) {
----------------
I'd explain what this function is supposed to do -- this function shifts `DataIndex` of tree nodes to fill a gap created by removing a default language node.


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

https://reviews.llvm.org/D66825