[PATCH] D36201: Merge manifest namespaces.
Reid Kleckner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 17 18:27:23 PDT 2017
rnk added a comment.
I think this is pretty close to ready. How do other reviewers feel?
================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:116
+// Search for prefix-defined namespace specified by HRef, starting on Node and
+// continuing recurisvely upwards. Returns the namespace or nullptr if not
+// found.
----------------
typo on "recursively"
================
Comment at: llvm/lib/WindowsManifest/WindowsManifestMerger.cpp:221
+ continue;
+ } else if (namespaceOverrides(OriginalAttribute->ns->href,
+ Attribute->ns->href)) {
----------------
ruiu wrote:
> ecbeckmann wrote:
> > ruiu wrote:
> > > Well, you want to remove `continue`, as I described in the previous comment. (Please fix not only this place but other places as well.)
> > wait I'm confused, I thought we want to add the early continues?
> Ah sorry, I meant not `continue` but `else`.
Most of these if / else branches are about equivalent in terms of complexity. I'd actually suggest removing all the continues that got added (except the `!Attribute->ns` check). Most of the continues in the current code are redundant, because they'd hit the end of the loop naturally anyway.
https://reviews.llvm.org/D36201
More information about the llvm-commits
mailing list