[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/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)) {
----------------
ecbeckmann wrote:
> 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.
What I was suggesting is adding something like this

  static bool streq(const unsigned char *A, const unsigned char B) {
    return strcmp((char *)A, (char *)B) == 0;
  }


================
Comment at: llvm/lib/Support/WindowsManifestMerger.cpp:19-20
 
+#define TO_XML_CHAR(X) reinterpret_cast<const unsigned char *>(X)
+#define FROM_XML_CHAR(X) reinterpret_cast<const char *>(X)
+
----------------
In C++ it is not a very good practice to use a C macro where you can avoid it. I'd probably keep the original reinterpret_casts, but if you like this, I think I'm fine with it.


https://reviews.llvm.org/D35753