Page MenuHomePhabricator

[clang-tidy] Add modernize-concat-nested-namespaces check
ClosedPublic

Authored by wgml on Sep 15 2018, 7:34 AM.

Details

Summary

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();
}

Diff Detail

Event Timeline

wgml created this revision.Sep 15 2018, 7:34 AM
lebedev.ri added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
74–75

It should only get registered in C++17 or newer mode.

lebedev.ri added inline comments.Sep 15 2018, 7:48 AM
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
43–46

Use proper casting, isa<>(), dyn_cast<>, so on.

80

Here and everywhere - the consts are on the wrong side.

103–109
  1. This is unrelated to the main check, so it should be in a separate check.
  2. This is really fragile i think. What about preprocessor-driven emptiness?
clang-tidy/modernize/ConcatNestedNamespacesCheck.h
30–33

This looks like a pessimization.
I think you should just store NamespaceDecl*.

Eugene.Zelenko added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
38

Type is not spelled in declaration, so please don't use auto.

54

Type is not spelled in declaration, so please don't use auto.

56

Type is not spelled in declaration, so please don't use auto.

docs/ReleaseNotes.rst
96

Wrong indentation. See other entries.

docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
13

Please add empty line after.

35

Please add empty line after.

Eugene.Zelenko added a project: Restricted Project.Sep 15 2018, 8:42 AM
wgml added inline comments.Sep 15 2018, 11:04 AM
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
103–109
  1. It seems to me that if I produce fixit to concat empty namespace (that is, execute the else branch), the fix is not applied as I intended - the namespace gets deleted.

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.

  1. Can you provide an example when it will matter?
wgml updated this revision to Diff 165658.Sep 15 2018, 12:16 PM
wgml marked 12 inline comments as done.

Adjusted to review comments.

clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
103–109

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.

wgml edited the summary of this revision. (Show Details)Sep 15 2018, 12:17 PM
wgml updated this revision to Diff 165677.Sep 16 2018, 5:33 AM

Fixed typo in documentation.
Refactored a bit of check code to make it more readable.

wgml updated this revision to Diff 165698.Sep 16 2018, 3:01 PM

One last typo.

alexfh added inline comments.Sep 17 2018, 2:52 AM
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
103–109

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.

wgml marked 2 inline comments as done.Sep 19 2018, 12:02 PM
wgml added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
103–109

That is good to know! Everything looks good to me then.

aaron.ballman added inline comments.Sep 20 2018, 2:11 PM
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
53

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).

74

Diagnostics should not start with a capital letter.

clang-tidy/modernize/ConcatNestedNamespacesCheck.h
30

Why 6?

docs/clang-tidy/checks/list.rst
14

This is an unrelated change -- feel free to make a separate commit that fixes this (no review needed).

wgml updated this revision to Diff 166529.Sep 21 2018, 11:57 AM
wgml marked 4 inline comments as done.
wgml edited the summary of this revision. (Show Details)
wgml marked 2 inline comments as done.
wgml added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
53

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.

clang-tidy/modernize/ConcatNestedNamespacesCheck.h
30

Educated guess. I considered the codebases I usually work with and it seemed a sane value in that context.

docs/clang-tidy/checks/list.rst
14

Setup script did that I guess. I reverted the change.

aaron.ballman added inline comments.Sep 21 2018, 5:46 PM
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
32

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.

51

I'm not keen on the trailing return type used here. It's reasonable, but clever in ways we don't use elsewhere.

53

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();
clang-tidy/modernize/ConcatNestedNamespacesCheck.h
30

Okay, I can live with that. Thanks!

wgml marked 6 inline comments as done.Sep 22 2018, 7:30 AM
wgml added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
32

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.

51

Ok. Wanted to avoid line break and the...

ConcatNestedNamespacesCheck::NamespaceString
ConcatNestedNamespacesCheck::concatNamespaces() {...}

but I'll adjust.

53

Yeah, I tried that, but Twine has it's operator= deleted.

wgml updated this revision to Diff 166602.Sep 22 2018, 7:33 AM
wgml marked an inline comment as done.

Updated signature of concatNamespaces.

aaron.ballman accepted this revision.Sep 24 2018, 1:55 PM

Aside from the local const qualification stuff and some minor wordsmithing of the documentation, this LGTM.

clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
32

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.

53

Ugh, good catch! The current formulation is fine then, thank you!

docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
7

in a form of -> such as

8

offers change to syntax -> suggests changing to the more concise syntax

9

Inlined -> Inline

This revision is now accepted and ready to land.Sep 24 2018, 1:55 PM
wgml updated this revision to Diff 166945.Sep 25 2018, 10:40 AM
wgml marked 10 inline comments as done.

Dropped a few consts, updated doc text.

wgml added a comment.Sep 25 2018, 10:43 AM

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?

clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
32

Okay then, adjusted.

Commited for you.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

Hi

Thanks a lot for this checker - would it be possible to enhance it to also update stuff in associated header files?

Thanks

Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 6:02 AM
wgml added a comment.Jan 28 2020, 1:49 PM

Hi

Thanks a lot for this checker - would it be possible to enhance it to also update stuff in associated header files?

Thanks

Check out D61989,

Abpostelnicu added inline comments.
clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
88

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.