[PATCH] D36201: Merge manifest namespaces.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/include/llvm/WindowsManifest/WindowsManifestMerger.h:54
 
+static const std::vector<std::pair<std::string, std::string>>
+    MtNsHrefsPrefixes = {
----------------
This will probably have internal linkage and get duplicated into every TU that includes this header. Can you sink this down into the .cpp file?

This is also just static data, so consider making this an array like this:
  static const std::pair<const char *, const char *> MtNsHrefsPrefixes[] = { .... };

That should avoid some dynamic initialization, while still letting you loop over it and make StringRefs: https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:35
+  // this indicates the prefix of a default namespace.
+  if (!(A && B))
+    return !A && !B;
----------------
I feel like this would be simpler as:
  // If either pointer is null, use pointer equality. If both are null, they are equal, otherwise they are unequal.
  if (!A || !B)
    return A == B;


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:42
 
 bool isMergeableElement(const unsigned char *ElementName) {
   for (StringRef S : {"application", "assembly", "assemblyIdentity",
----------------
Right now, these functions are externally visible symbols in the global namespace. We should take some steps to hide them from users of LLVM. Any free function that you don't need outside this file should be static: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:145
+                             XMLAttrImpl AdditionalAttribute) {
+#if LLVM_LIBXML2_ENABLED
+  Expected<XMLNsImpl> ExplicitOrError =
----------------
Echoing what @ruiu said, I was imagining that we'd have one large ifdef between stub implementations of WindowsManifest::merge etc, and the real implementations and all of the helpers you wrote here that call libxml2.


https://reviews.llvm.org/D36201