[PATCH] D35753: llvm-mt: implement simple merging of manifests, not factoring namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:38
 
+static const char *const MT_Strings[]{
+    "application",         "assembly",  "assemblyIdentity",
----------------
Even if C++11 allows this style, I think inserting `=` like this is more common.

  MT_String[] = {


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:70
 public:
+#if LLVM_LIBXML2_ENABLED
+  ~WindowsManifestMerger();
----------------
Conditionally defining a dtor seems tricky. It is probably better to define this regardless of the presence of libxml and do nothing if libxml is not available.


================
Comment at: llvm/include/llvm/Support/WindowsManifestMerger.h:84
 
 #if LLVM_LIBXML2_ENABLED
+  XMLDocumentImpl CombinedDoc = nullptr;
----------------
Likewise, always define this variable and don't use it if libxml is not available.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:29
+                                           const Twine &Message)
+    : Msg((Prefix.concat(Message)).str()) {}
+
----------------
`Prefix + Message`?


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:34
+bool isMergeableElement(const unsigned char *ElementName) {
+  bool result =
+      std::find(MTMergeableElements.begin(), MTMergeableElements.end(),
----------------
I think a better way of writing this without a global variable is this.

  for (StringRef S : {"application",         "assembly",  "assemblyIdentity",
                      "compatibility",       "noInherit", "requestedExecutionLevel",
                      "requestedPrivileges", "security",  "trustInfo"}) {
    if (S == ElementName)
      return true;
  }
  return false;


https://reviews.llvm.org/D35753