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

[Nico Weber via Phabricator via llvm-commits]

thakis added inline comments.


================
Comment at: llvm/include/llvm/Object/WindowsResource.h:160
               std::vector<std::string> &Duplicates);
+  void cleanupManifests(std::vector<std::string> &Duplicates);
   void printTree(raw_ostream &OS) const;
----------------
nit: cleanUpManifests – the verb is "to clean up" (with "cleanup" is the corresponding noun)


================
Comment at: llvm/lib/Object/WindowsResource.cpp:310
+// the function comment above for context.
+bool WindowsResourceParser::ignoreDuplicate(const ResourceEntryRef &Entry) {
+  return !Entry.checkTypeString() &&
----------------
nit: call "shouldIgnoreDuplicate" and make `const`; as-is the function sounds like it modifies state


================
Comment at: llvm/lib/Object/WindowsResource.cpp:319
+bool WindowsResourceParser::ignoreDuplicate(
+    const std::vector<StringOrID> &Context) {
+  return Context.size() == 3 && !Context[0].IsString &&
----------------
same


================
Comment at: llvm/lib/Object/WindowsResource.cpp:353
     if (!IsNewNode) {
-      Duplicates.push_back(makeDuplicateResourceError(
-          Entry, InputFilenames[Node->Origin], WR->getFileName()));
+      if (!ignoreDuplicate(Entry))
+        Duplicates.push_back(makeDuplicateResourceError(
----------------
Do we have to add code for this to the merging logic? Or is the implicit manifest always res obj file 0 and we can skip that if there are more than 1 res obj files?


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

https://reviews.llvm.org/D66825