[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.

I think it is fine to make this logic run unconditionally if the logic doesn't harm non-mingw use cases.



================
Comment at: llvm/lib/Object/WindowsResource.cpp:254
 
+bool WindowsResourceParser::checkManifestChildren(
+    TreeNode &Node, uint32_t Lang, uint32_t &DataIndex,
----------------
Can you add a function comment?


================
Comment at: llvm/lib/Object/WindowsResource.cpp:266
+    return false;
+  } else if (PrevLang == 0) {
+    // Already have a lang=0 manifest, replace it with the new one,
----------------
nit: no `else` after `return`.


================
Comment at: llvm/lib/Object/WindowsResource.cpp:307
     uint32_t DataIndex = Data.size();
+    if (!Entry.checkTypeString() && Entry.getTypeID() == /* RT_MANIFEST */ 24 &&
+        !Entry.checkNameString() &&
----------------
Detecting duplicate manifest objects having the same ID and skipping duplicate zero language IDs while reading an object file seems a bit tricky. Do you think you can do after reading the entire manifest file? If you can, you can separate the logic to parse a file from the logic that you want to add in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66825