hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land.
Sorry, I missed this patch. LGTM with one nit. ================ Comment at: change-namespace/ChangeNamespace.cpp:296 + assert(!NsSplitted.empty()); + for (auto I = NsSplitted.begin() + 1, E = NsSplitted.end(); I != E; ++I) { + if (*I == SymbolSplitted.front()) ---------------- ioeric wrote: > hokein wrote: > > Why skipping the first element? And use `is_contained` instead? > See newly added comments for reasoning. I see. This sounds the `conflictInNamespace` is too coupled with caller because it relies on "it equals to the symbol's outermost namespace and the symbol name would have been shortened" assumption. It is not straightforward especially for readers who read the code at the first time. So I'd like to search from 0 (and this operation is trivial). https://reviews.llvm.org/D30493 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits