[PATCH] D36201: Merge manifest namespaces.

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:274-276
+  else if (!Node2 || !Node2->ns)
+    return Node1;
+  else if (namespaceOverrides(Node1->ns->href, Node2->ns->href))
----------------
No `else` after `return`


================
Comment at: llvm/test/tools/llvm-mt/big_merge.test:4-7
+RUN: llvm-mt /manifest %p/Inputs/trust_info.manifest /manifest \
+RUN:   %p/Inputs/assembly_identity.manifest /manifest \
+RUN:   %p/Inputs/trust_and_identity.manifest /manifest \
+RUN:   %p/Inputs/compatibility.manifest /manifest \
----------------
Maybe it is better to write

   /manifest <path1> \
   /manifest <path2> \
   ...

instead of

  <path1> /manifest \
  <path2> /manifest \
   ...

because paths are arguments for /manifest.


================
Comment at: llvm/tools/llvm-mt/llvm-mt.cpp:106-107
+  for (auto &Arg : InputArgs) {
+    if (!(Arg->getOption().matches(OPT_unsupported) ||
+          Arg->getOption().matches(OPT_supported))) {
+      reportError(Twine("invalid option ") + Arg->getSpelling());
----------------
`!A && !B` is preferred over `!(A || B)`.

But do you have to do this way? I think usually we handle OPT_UNKNOWN to report unknown options instead of putting all known options to some groups. Look at https://github.com/llvm-project/llvm-project-20170507/blob/dd24779ef3e3e592cc0c7561a2fb1fbe3309fa1c/lld/ELF/DriverUtils.cpp#L112 for example.


https://reviews.llvm.org/D36201