[PATCH] D35753: llvm-mt: implement simple merging of manifests, not factoring namespaces.
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 24 16:30:18 PDT 2017
ruiu added inline comments.
================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:52
WindowsManifestError(const Twine &Msg);
+ WindowsManifestError(const Twine &Prefix, const Twine &Message);
void log(raw_ostream &OS) const override;
----------------
Why do you need this? Looks like you can just concatenate a string before passing it to the ctor.
================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:34
+bool isMergeableElement(const unsigned char *ElementName) {
+for (StringRef S : {"application", "assembly", "assemblyIdentity",
+ "compatibility", "noInherit", "requestedExecutionLevel",
----------------
Format
================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:99
+ XMLNodeImpl OriginalChildWithName;
+ if ((strcmp(reinterpret_cast<const char *>(Child->name), "text") != 0) &&
+ (strcmp(reinterpret_cast<const char *>(Child->name), "comment") !=
----------------
Do you need this reinterpret_cast? It looks like Child->name is of `xmlChar *` which is a typedef to `unsigned char *`.
================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:99
+ XMLNodeImpl OriginalChildWithName;
+ if ((strcmp(reinterpret_cast<const char *>(Child->name), "text") != 0) &&
+ (strcmp(reinterpret_cast<const char *>(Child->name), "comment") !=
----------------
ruiu wrote:
> Do you need this reinterpret_cast? It looks like Child->name is of `xmlChar *` which is a typedef to `unsigned char *`.
Reverse the condition so that you can `continue` early.
================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:110-111
+ xmlAddChild(OriginalRoot, NewChild);
+ } else {
+ if (auto E = treeMerge(OriginalChildWithName, Child))
+ return E;
----------------
`else if` instead of `else { if ... }`.
================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:126
+#if LLVM_LIBXML2_ENABLED
+ for(auto& Doc : MergedDocs)
+ xmlFreeDoc(Doc);
----------------
Format
https://reviews.llvm.org/D35753
More information about the llvm-commits
mailing list