aaron.ballman added inline comments.
================
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)
----------------
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.
================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:50
+
+auto ConcatNestedNamespacesCheck::concatNamespaces() -> NamespaceString {
+ NamespaceString Result("namespace ");
----------------
I'm not keen on the trailing return type used here. It's reasonable, but clever
in ways we don't use elsewhere.
================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+ const NamespaceContextVec &Namespaces) {
+ std::ostringstream Result;
+ bool First = true;
----------------
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();
```
================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29
+private:
+ using NamespaceContextVec = llvm::SmallVector<const NamespaceDecl *, 6>;
+
----------------
wgml wrote:
> aaron.ballman wrote:
> > Why 6?
> Educated guess. I considered the codebases I usually work with and it seemed
> a sane value in that context.
Okay, I can live with that. Thanks!
https://reviews.llvm.org/D52136
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
