wgml marked 6 inline comments as done. wgml added inline comments.
================ Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52 + const NamespaceContextVec &Namespaces) { + std::ostringstream Result; + bool First = true; ---------------- aaron.ballman wrote: > wgml wrote: > > aaron.ballman wrote: > > > Can this be rewritten with `llvm::for_each()` and a `Twine` so that we > > > don't have to use `ostringstream` (which is a big hammer for this). > > The main advantage of `stringstream` was that it is concise. > > > > I don't think I can effectively use `Twine` to build a result in a loop. If > > I'm wrong, correct me, please. > > > > I reworked `concatNamespaces` to use `SmallString` with another educated > > guess of `40` for capacity. > The `Twine` idea I was thinking of is not too far off from what you have with > `SmallString`, but perhaps is too clever: > ``` > return std::accumulate(Namespaces.begin(), Namespaces.end(), llvm::Twine(), > [](llvm::Twine Ret, const NamespaceDecl *ND) { > return Ret + "::" + ND->getName(); > }).str(); > ``` Yeah, I tried that, but Twine has it's `operator=` deleted. ================ Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:31 +static bool singleNamedNamespaceChild(const NamespaceDecl &ND) { + const NamespaceDecl::decl_range Decls = ND.decls(); + if (std::distance(Decls.begin(), Decls.end()) != 1) ---------------- aaron.ballman wrote: > We usually only const-qualify local declarations when they're > pointers/references, so you can drop the `const` here (and in several other > places). It's not a hard and fast rule, but the clutter is not useful in such > small functions. From my perspective, `const` is a easy way of declaring that my intention is only to name given declaration only for reading and to improve code readability. ================ Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:50 + +auto ConcatNestedNamespacesCheck::concatNamespaces() -> NamespaceString { + NamespaceString Result("namespace "); ---------------- aaron.ballman wrote: > I'm not keen on the trailing return type used here. It's reasonable, but > clever in ways we don't use elsewhere. Ok. Wanted to avoid line break and the... ``` ConcatNestedNamespacesCheck::NamespaceString ConcatNestedNamespacesCheck::concatNamespaces() {...} ``` but I'll adjust. https://reviews.llvm.org/D52136 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits