[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
llvm-commits at lists.llvm.org
Tue Sep 3 10:34:27 PDT 2019
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
More information about the llvm-commits
mailing list