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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 4 inline comments as done.
mstorsjo added inline comments.


================
Comment at: lld/test/COFF/merge-resource-manifest.test:12
+
+# RUN: lld-link /lldmingw /out:%t.exe /entry:main %t.obj %p/Inputs/manifest-lang1.o %p/Inputs/manifest-lang0.o
+# RUN: llvm-readobj --coff-resources %t.exe | FileCheck %s
----------------
MaskRay wrote:
> Does yaml2obj lack any features that prevents it from generating `manifest-lang0.o` and other object files?
Hmm, no, yaml2obj should work fine.

In principle we could also produce them with llvm-rc and llvm-cvtres, but we currently don't have those as test-depends for lld. (And for patch 09/10 I have object files generated with other tools, to test interoperability with them. But a yaml serialization of it should preserve all the necessary detail.


================
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
----------------
ruiu wrote:
> 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.
Ok, will expand the comment. Currently the explanation is mostly in the commit message, but I'll try to make the individual comments also understandable standalone.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:282
+    return;
+  // More than one non-language-zero manifest
+  auto FirstIt = NameNode->IDChildren.begin();
----------------
ruiu wrote:
> 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.
Sure, will do.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:562
 
+void WindowsResourceParser::TreeNode::shiftDataIndexDown(uint32_t Index) {
+  if (IsDataNode && DataIndex >= Index) {
----------------
ruiu wrote:
> 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.
Ah yes, will add a comment here as well. 


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

https://reviews.llvm.org/D66825