Page MenuHomePhabricator

[clang-tidy] Fix google-readability-namespace-comments handling of C++17 nested namespaces
ClosedPublic

Authored by predator5047 on Sep 26 2017, 11:07 AM.

Details

Summary

Fixes bug 34701

When we encounter a namespace find the location of the left bracket.
Then if the text between the name and the left bracket contains a ':'
then it's a C++17 nested namespace.

Diff Detail

Repository
rL LLVM

Event Timeline

predator5047 created this revision.Sep 26 2017, 11:07 AM

Could you add a test case for nested namespaces? I think it should be in a new file with C++17 enabled.

krasimir added inline comments.
clang-tidy/readability/NamespaceCommentCheck.cpp
60 ↗(On Diff #116689)

Why there is an n at the end of namespacen?

Fixed spelling mistake and added a test case.

aaron.ballman added inline comments.Sep 28 2017, 1:21 PM
clang-tidy/readability/NamespaceCommentCheck.cpp
86 ↗(On Diff #116850)

Do not use auto here (the type is not spelled out in the initialization, despite being relatively obvious. Same below.

90 ↗(On Diff #116850)

Missing a period at the end of the sentence.

91 ↗(On Diff #116850)

i does not meet the usual naming conventions; you might want to pick a slightly better name.

92 ↗(On Diff #116850)

You can elide the braces from the if statement.

109 ↗(On Diff #116850)

Elide braces

137 ↗(On Diff #116850)

Missing period in the comment

140–141 ↗(On Diff #116850)

I'd move the comments inside of the braces (the braces can be elided, but if the comments are in the body instead of out of it, I think they're fine to leave).

clang-tidy/readability/NamespaceCommentCheck.h
37 ↗(On Diff #116850)

Why 7?

test/clang-tidy/google-readability-nested-namespace-comments.cpp
6 ↗(On Diff #116850)

Comment should start with a capital letter and end with punctuation.

8–14 ↗(On Diff #116850)

Can remove the extra whitespace

23 ↗(On Diff #116850)

Please make sure there's a newline at the end of the file.

predator5047 marked 12 inline comments as done.

Address review comments:

  • Don't use auto if the type is not spelled out in initialization.
  • Better names for variables.
  • Elide braces.
  • Format changes.
aaron.ballman added inline comments.Oct 2 2017, 9:29 AM
clang-tidy/readability/NamespaceCommentCheck.cpp
59 ↗(On Diff #117193)

s should be renamed to S or something more descriptive that meets the coding guidelines.

92 ↗(On Diff #117193)

While you *can* elide the braces for the for loop, I think it looks a bit strange (we usually do it for if statements but not loops). You might want to put the braces back around the for loop body.

114 ↗(On Diff #117193)

Same here.

clang-tidy/readability/NamespaceCommentCheck.h
37 ↗(On Diff #117193)

I thought your use of SmallVector was appropriate, I was just wondering about the use of 7 as the expected size. I would have imagined we could get away with 2-4.

test/clang-tidy/google-readability-nested-namespace-comments.cpp
6 ↗(On Diff #117193)

Space between // and the start of the comment.

predator5047 marked 5 inline comments as done.

Address review comments:

  • Use llvm::SmallVector instead of std::vector
  • Some formatting changes.
aaron.ballman accepted this revision.Oct 4 2017, 6:03 AM

Aside from a small nit, this LGTM, thanks!

clang-tidy/readability/NamespaceCommentCheck.cpp
102–105 ↗(On Diff #117398)

These should not use auto since the type is not explicitly spelled out in the initialization.

This revision is now accepted and ready to land.Oct 4 2017, 6:03 AM

Aside from a small nit, this LGTM, thanks!

Can you commit it? I don't have commit rights.

aaron.ballman closed this revision.Oct 6 2017, 5:59 AM

I've commit in r315057. Thank you!

alexfh added inline comments.Oct 12 2017, 7:06 AM
clang-tidy/readability/NamespaceCommentCheck.cpp
97 ↗(On Diff #117398)

The check started triggering an assertion failure and incredible slowness (infinite loops?) on some real files. I've not yet come up with an isolated test case, but all this seems to be happening around this loop.

I'm going to revert this revision. A bug report (and hopefully a test case ;) will follow.

alexfh added inline comments.Oct 12 2017, 7:25 AM
clang-tidy/readability/NamespaceCommentCheck.cpp
97 ↗(On Diff #117398)

Reverted in r315580.

alexfh added inline comments.Oct 13 2017, 6:14 AM
clang-tidy/readability/NamespaceCommentCheck.cpp
97 ↗(On Diff #117398)

Here's a test case that demonstrates the issue:

#define MACRO macro_expansion
namespace MACRO {
void f(); // So that the namespace isn't empty.
// 1
// 2
// 3
// 4
// 5
// 6
// 7
// CHECK-MESSAGES: :[[@LINE+2]]:2: warning: namespace 'macro_expansion' not terminated with
// CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'macro_expansion' starts here
}
// CHECK-FIXES: }  // namespace macro_expansion

I'll commit it once Subversion on llvm.org starts working again.

alexfh added inline comments.Oct 13 2017, 7:24 AM
clang-tidy/readability/NamespaceCommentCheck.cpp
97 ↗(On Diff #117398)

Committed the regression test in r315682.

Fixed assert error caused by macros.

It looks like the latest patch was lost. I'll see whether it still applies cleanly...

alexfh reopened this revision.Jan 10 2018, 8:31 AM
This revision is now accepted and ready to land.Jan 10 2018, 8:31 AM
This revision was automatically updated to reflect the committed changes.