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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:77-79
+      char *NameCopy = strdup(reinterpret_cast<const char *>(Attribute->name));
+      char *ContentCopy =
+          strdup(reinterpret_cast<const char *>(Attribute->children->content));
----------------
ruiu wrote:
> The memory you allocate using strdup needs to be freed somewhere using free(). Does your patch do that?
Yes,  these strings become part of the node tree structure of the xml document.  When xmlFreeDoc() is called by the class destructor these strings should also be freed.


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:148-149
+    XMLNodeImpl AdditionalRoot = xmlDocGetRootElement(ManifestXML);
+    if (strcmp(reinterpret_cast<const char *>(CombinedRoot->name),
+               reinterpret_cast<const char *>(AdditionalRoot->name)) == 0 &&
+        isMergeableElement(AdditionalRoot->name)) {
----------------
ruiu wrote:
> I think we have too many occurrences of this pattern. Can you add a function that compares two xmlStrings?
All the function would do is abstract away the reinterpret_cast.  I think with the macro I added for cast it isn't that bad.


https://reviews.llvm.org/D35753