[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check
Wojtek GumuĊa via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Sep 22 07:33:03 PDT 2018
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
More information about the cfe-commits
mailing list