Finds instances of namespaces concatenated using explicit syntax, such as namespace a { namespace b { [...] }} and offers fix to glue it to namespace a::b { [...] }.
Properly handles inline and unnamed namespaces. Also, detects empty blocks in nested namespaces and offers to remove them.
Test with common use cases included.
I ran the check against entire llvm repository. Except for expected nested namespace definitions only available with -std=c++17 or -std=gnu++17 warnings I noticed no issues when the check was performed.
Example:
namespace a { namespace b {
void test();
}}
can become
namespace a::b {
void test();
}
It should only get registered in C++17 or newer mode.
Use proper casting, isa<>(), dyn_cast<>, so on.
Here and everywhere - the consts are on the wrong side.
This looks like a pessimization.
I think you should just store NamespaceDecl*.
Type is not spelled in declaration, so please don't use auto.
Wrong indentation. See other entries.
Please add empty line after.
I do not fully understand this behavior and did not manage to find bug in the code. I also checked what will happen if I try replace the entire namespace with namespace {} and it is not getting inserted but simply deleted. On the other hand, I if replace with namespace {;}, I see the expected output. So, either it is really sneaky off-by-one somewhere, or clang does it's own thing.
Truth be told, I added that if branch to handle such behavior explicitly instead of silently deleting namespace.
Adjusted to review comments.
I dropped the "support" for removing. However, empty namespaces are still being removed implicitly, by a fixit applying tool, I believe.
I added entries to the test to make sure preprocessor stuff is never removed.
Fixed typo in documentation.
Refactored a bit of check code to make it more readable.
One last typo.
Removal of empty namespaces is intentional. Clang-tidy calls clang::format::cleanupAroundReplacements when applying fixes, which, in particular, removes empty namespaces. See code around clang/lib/Format/Format.cpp:1309. The motivation is that when different checks remove different parts of code inside a namespace and this results in an empty namespace, it's better to remove it. Other cleanups are, for example, removal of commas and the colon in constructor initializer lists.
That is good to know! Everything looks good to me then.
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).
Diagnostics should not start with a capital letter.
Why 6?
This is an unrelated change -- feel free to make a separate commit that fixes this (no review needed).
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.
Educated guess. I considered the codebases I usually work with and it seemed a sane value in that context.
Setup script did that I guess. I reverted the change.
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.
I'm not keen on the trailing return type used here. It's reasonable, but clever in ways we don't use elsewhere.
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();
Okay, I can live with that. Thanks!
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.
Ok. Wanted to avoid line break and the...
ConcatNestedNamespacesCheck::NamespaceString
ConcatNestedNamespacesCheck::concatNamespaces() {...}
but I'll adjust.
Yeah, I tried that, but Twine has it's operator= deleted.
Updated signature of concatNamespaces.
Aside from the local const qualification stuff and some minor wordsmithing of the documentation, this LGTM.
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.
Ugh, good catch! The current formulation is fine then, thank you!
in a form of -> such as
offers change to syntax -> suggests changing to the more concise syntax
Inlined -> Inline
Dropped a few consts, updated doc text.
Thanks you all for your participation in the review process. I guess it is done now.
I don't have write access to the repositories, @aaron.ballman, could you apply the patch on my behalf?
Okay then, adjusted.
Commited for you.
Hi
Thanks a lot for this checker - would it be possible to enhance it to also update stuff in associated header files?
Thanks
In D52136#1844384, @grandinj wrote:
Hi
Thanks a lot for this checker - would it be possible to enhance it to also update stuff in associated header files?
Thanks
This breaks the usage of this checker in an unified build scenario since the main file for the translation unit will be the unified file.