aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
Aside from the local `const` qualification stuff and some minor wordsmithing of
the documentation, this LGTM.
================
Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+ const NamespaceContextVec &Namespaces) {
+ std::ostringstream Result;
+ bool First = true;
----------------
wgml wrote:
> 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.
Ugh, good catch! The current formulation is fine then, thank you!
================
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)
----------------
wgml wrote:
> 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.
I wasn't making a value judgement about the coding style so much as pointing
out that this is novel to the code base and we tend to avoid novel constructs
unless there's a good reason to deviate. I don't see a strong justification
here, so I'd prefer them to be removed for consistency.
================
Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:6
+
+Checks for use of nested namespaces in a form of ``namespace a { namespace b {
... } }``
+and offers change to syntax introduced in C++17: ``namespace a::b { ... }``.
----------------
in a form of -> such as
================
Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:7
+Checks for use of nested namespaces in a form of ``namespace a { namespace b {
... } }``
+and offers change to syntax introduced in C++17: ``namespace a::b { ... }``.
+Inlined namespaces are not modified.
----------------
offers change to syntax -> suggests changing to the more concise syntax
================
Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:8
+and offers change to syntax introduced in C++17: ``namespace a::b { ... }``.
+Inlined namespaces are not modified.
+
----------------
Inlined -> Inline
https://reviews.llvm.org/D52136
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
