[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Andi via Phabricator via cfe-commits]

Abpostelnicu added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:88
+
+  if (!Sources.isInMainFile(ND.getBeginLoc()))
+return;

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. 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52136/new/

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml added a comment.

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


Check out D61989 ,


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52136/new/

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Noel Grandin via Phabricator via cfe-commits]

grandinj added a comment.
Herald added a project: LLVM.

Hi

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

Thanks


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52136/new/

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343000: [clang-tidy] Add modernize-concat-nested-namespaces 
check (authored by JonasToth, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52136?vs=166945=166959#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52136

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -0,0 +1,113 @@
+//===--- ConcatNestedNamespacesCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ConcatNestedNamespacesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include 
+#include 
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+static bool locationsInSameFile(const SourceManager ,
+SourceLocation Loc1, SourceLocation Loc2) {
+  return Loc1.isFileID() && Loc2.isFileID() &&
+ Sources.getFileID(Loc1) == Sources.getFileID(Loc2);
+}
+
+static bool anonymousOrInlineNamespace(const NamespaceDecl ) {
+  return ND.isAnonymousNamespace() || ND.isInlineNamespace();
+}
+
+static bool singleNamedNamespaceChild(const NamespaceDecl ) {
+  NamespaceDecl::decl_range Decls = ND.decls();
+  if (std::distance(Decls.begin(), Decls.end()) != 1)
+return false;
+
+  const auto *ChildNamespace = dyn_cast(*Decls.begin());
+  return ChildNamespace && !anonymousOrInlineNamespace(*ChildNamespace);
+}
+
+static bool alreadyConcatenated(std::size_t NumCandidates,
+const SourceRange ,
+const SourceManager ,
+const LangOptions ) {
+  CharSourceRange TextRange =
+  Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
+  StringRef CurrentNamespacesText =
+  Lexer::getSourceText(TextRange, Sources, LangOpts);
+  return CurrentNamespacesText.count(':') == (NumCandidates - 1) * 2;
+}
+
+ConcatNestedNamespacesCheck::NamespaceString
+ConcatNestedNamespacesCheck::concatNamespaces() {
+  NamespaceString Result("namespace ");
+  Result.append(Namespaces.front()->getName());
+
+  std::for_each(std::next(Namespaces.begin()), Namespaces.end(),
+[](const NamespaceDecl *ND) {
+  Result.append("::");
+  Result.append(ND->getName());
+});
+
+  return Result;
+}
+
+void ConcatNestedNamespacesCheck::registerMatchers(
+ast_matchers::MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus17)
+return;
+
+  Finder->addMatcher(ast_matchers::namespaceDecl().bind("namespace"), this);
+}
+
+void ConcatNestedNamespacesCheck::reportDiagnostic(
+const SourceRange , const SourceRange ) {
+  diag(Namespaces.front()->getBeginLoc(),
+   "nested namespaces can be concatenated", DiagnosticIDs::Warning)
+  << FixItHint::CreateReplacement(FrontReplacement, concatNamespaces())
+  << FixItHint::CreateReplacement(BackReplacement, "}");
+}
+
+void ConcatNestedNamespacesCheck::check(
+const ast_matchers::MatchFinder::MatchResult ) {
+  const NamespaceDecl  = *Result.Nodes.getNodeAs("namespace");
+  const SourceManager  = *Result.SourceManager;
+
+  if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
+return;
+
+  if (!Sources.isInMainFile(ND.getBeginLoc()))
+return;
+
+  if (anonymousOrInlineNamespace(ND))
+return;
+
+  Namespaces.push_back();
+
+  if (singleNamedNamespaceChild(ND))
+return;
+
+  SourceRange FrontReplacement(Namespaces.front()->getBeginLoc(),
+   Namespaces.back()->getLocation());
+  SourceRange BackReplacement(Namespaces.back()->getRBraceLoc(),
+  Namespaces.front()->getRBraceLoc());
+
+  if (!alreadyConcatenated(Namespaces.size(), FrontReplacement, Sources,
+   getLangOpts()))
+reportDiagnostic(FrontReplacement, 

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE343000: [clang-tidy] Add 
modernize-concat-nested-namespaces check (authored by JonasToth, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- test/clang-tidy/modernize-concat-nested-namespaces.cpp
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: clang-tidy/modernize/CMakeLists.txt
===
--- clang-tidy/modernize/CMakeLists.txt
+++ clang-tidy/modernize/CMakeLists.txt
@@ -2,6 +2,7 @@
 
 add_clang_library(clangTidyModernizeModule
   AvoidBindCheck.cpp
+  ConcatNestedNamespacesCheck.cpp
   DeprecatedHeadersCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
Index: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
===
--- clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
+++ clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
@@ -0,0 +1,113 @@
+//===--- ConcatNestedNamespacesCheck.cpp - clang-tidy--===//
+//

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Commited for you.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml added a comment.

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?




Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:31
+static bool singleNamedNamespaceChild(const NamespaceDecl ) {
+  const NamespaceDecl::decl_range Decls = ND.decls();
+  if (std::distance(Decls.begin(), Decls.end()) != 1)

aaron.ballman wrote:
> 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.
Okay then, adjusted.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 166945.
wgml marked 10 inline comments as done.
wgml added a comment.

Dropped a few consts, updated doc text.


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+Checks for use of nested namespaces such as ``namespace a { namespace b { ... } }``
+and suggests changing to the more concise syntax introduced in C++17: ``namespace a::b { ... }``.
+Inline namespaces are not modified.
+
+For example:
+
+.. code-block:: c++
+
+  namespace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Aaron Ballman via Phabricator via cfe-commits]

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 ) {
+  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 ) {
+  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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 166602.
wgml marked an inline comment as done.
wgml added a comment.

Updated signature of `concatNamespaces`.


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+
+  namespace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. 

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml marked 6 inline comments as done.
wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+const NamespaceContextVec ) {
+  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 ) {
+  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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:31
+static bool singleNamedNamespaceChild(const NamespaceDecl ) {
+  const NamespaceDecl::decl_range Decls = ND.decls();
+  if (std::distance(Decls.begin(), Decls.end()) != 1)

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.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:50
+
+auto ConcatNestedNamespacesCheck::concatNamespaces() -> NamespaceString {
+  NamespaceString Result("namespace ");

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+const NamespaceContextVec ) {
+  std::ostringstream Result;
+  bool First = true;

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29
+private:
+  using NamespaceContextVec = llvm::SmallVector;
+

wgml wrote:
> aaron.ballman wrote:
> > Why 6?
> Educated guess. I considered the codebases I usually work with and it seemed 
> a sane value in that context.
Okay, I can live with that. Thanks!


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml marked 2 inline comments as done.
wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+const NamespaceContextVec ) {
+  std::ostringstream Result;
+  bool First = true;

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.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29
+private:
+  using NamespaceContextVec = llvm::SmallVector;
+

aaron.ballman wrote:
> Why 6?
Educated guess. I considered the codebases I usually work with and it seemed a 
sane value in that context.



Comment at: docs/clang-tidy/checks/list.rst:13
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept

aaron.ballman wrote:
> This is an unrelated change -- feel free to make a separate commit that fixes 
> this (no review needed).
Setup script did that I guess. I reverted the change.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 166529.
wgml marked 4 inline comments as done.
wgml edited the summary of this revision.

https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+
+  namespace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. code-block:: c++
+
+  namespace 

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52
+const NamespaceContextVec ) {
+  std::ostringstream Result;
+  bool First = true;

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:73
+  diag(Namespaces.front()->getBeginLoc(),
+   "Nested namespaces can be concatenated", DiagnosticIDs::Warning)
+  << FixItHint::CreateReplacement(FrontReplacement,

Diagnostics should not start with a capital letter.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29
+private:
+  using NamespaceContextVec = llvm::SmallVector;
+

Why 6?



Comment at: docs/clang-tidy/checks/list.rst:13
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept

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


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml marked 2 inline comments as done.
wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

alexfh wrote:
> wgml wrote:
> > wgml wrote:
> > > lebedev.ri wrote:
> > > > 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?
> > > 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.
> > > 
> > > 2. Can you provide an example when it will matter?
> > 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.
> 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.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

wgml wrote:
> wgml wrote:
> > lebedev.ri wrote:
> > > 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?
> > 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.
> > 
> > 2. Can you provide an example when it will matter?
> 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.
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.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 165698.
wgml added a comment.

One last typo.


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+
+  namespace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. code-block:: c++
+
+  namespace n1::n2 {
+  void t();
+  }
+
+  namespace 

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 165677.
wgml added a comment.

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


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+
+  namespace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. 

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml updated this revision to Diff 165658.
wgml marked 12 inline comments as done.
wgml added a comment.

Adjusted to review comments.


https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,161 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 {
+void t();
+}
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 {
+void t();
+}
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n31
+// CHECK-FIXES-EMPTY
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+} // namespace n34
+// CHECK-FIXES-EMPTY
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+#define IEXIST
+namespace n39 {
+namespace n40 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n39::n40
+#ifdef IEXIST
+void t() {}
+#endif
+} // namespace n40
+} // namespace n39
+// CHECK-FIXES: }
+
+namespace n41 {
+namespace n42 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n41::n42
+#ifdef IDONTEXIST
+void t() {}
+#endif
+} // namespace n42
+} // namespace n41
+// CHECK-FIXES: }
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+#ifdef IEXIST
+  n39::n40::t();
+#endif
+
+#ifdef IDONTEXIST
+  n41::n42::t();
+#endif
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,49 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+
+  namspace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+.. code-block:: c++
+
+  

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

wgml wrote:
> lebedev.ri wrote:
> > 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?
> 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.
> 
> 2. Can you provide an example when it will matter?
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.


https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

lebedev.ri wrote:
> 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?
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.

2. Can you provide an example when it will matter?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:37
+static bool singleNamedNamespaceChild(NamespaceDecl const ) {
+  auto const Decls = ND.decls();
+  if (childrenCount(Decls) != 1)

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:53
+LangOptions const ) {
+  auto const TextRange =
+  Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:55
+  Lexer::getAsCharRange(ReplacementRange, Sources, LangOpts);
+  auto const CurrentNamespacesText =
+  Lexer::getSourceText(TextRange, Sources, LangOpts);

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



Comment at: docs/ReleaseNotes.rst:96
 
+  - New :doc:`modernize-concat-nested-namespaces
+  ` check.

Wrong indentation. See other entries.



Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:12
+
+.. code-block:: c++
+  namspace n1 {

Please add empty line after.



Comment at: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst:34
+
+.. code-block:: c++
+  namspace n1::n2 {

Please add empty line after.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:42-45
+  if (Decl->getKind() != Decl::Kind::Namespace)
+return false;
+
+  auto const *NS = reinterpret_cast(Decl);

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:79
+const MatchFinder::MatchResult ) {
+  NamespaceDecl const  = 
*Result.Nodes.getNodeAs("namespace");
+  SourceManager const  = *Result.SourceManager;

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



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:102-108
+  if (childrenCount(ND.decls()) == 0) {
+SourceRange Removal(Namespaces.front().Begin, Namespaces.front().RBrace);
+
+diag(Namespaces.front().Begin,
+ "Nested namespaces are empty and can be removed",
+ DiagnosticIDs::Warning)
+<< FixItHint::CreateRemoval(Removal);

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?



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.h:29-32
+  struct NamespaceContext {
+std::string Name;
+SourceLocation Begin, NameBegin, RBrace;
+  };

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:73-74
+void ConcatNestedNamespacesCheck::registerMatchers(MatchFinder *Finder) {
+  if (getLangOpts().CPlusPlus)
+Finder->addMatcher(namespaceDecl().bind("namespace"), this);
+}

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

[Wojtek Gumuła via Phabricator via cfe-commits]

wgml created this revision.
wgml added reviewers: alexfh, aaron.ballman, hokein.
Herald added subscribers: xazax.hun, mgorny.

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();
  }}
  
  namespace c { namespace d { namespace e { }}}

can become

  namespace a::b {
  void test();
  }




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52136

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp
  clang-tidy/modernize/ConcatNestedNamespacesCheck.h
  clang-tidy/modernize/ModernizeTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
  test/clang-tidy/modernize-concat-nested-namespaces.cpp

Index: test/clang-tidy/modernize-concat-nested-namespaces.cpp
===
--- /dev/null
+++ test/clang-tidy/modernize-concat-nested-namespaces.cpp
@@ -0,0 +1,124 @@
+// RUN: %check_clang_tidy %s modernize-concat-nested-namespaces %t -- -- -std=c++17
+
+namespace n1 {}
+
+namespace n2 {
+namespace n3 {
+void t();
+}
+namespace n4 {
+void t();
+}
+} // namespace n2
+
+namespace n5 {
+inline namespace n6 { void t(); }
+} // namespace n5
+
+namespace n7 {
+void t();
+
+namespace n8 {
+void t();
+}
+} // namespace n7
+
+namespace n9 {
+namespace n10 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n9::n10
+void t();
+} // namespace n10
+} // namespace n9
+// CHECK-FIXES: }
+
+namespace n11 {
+namespace n12 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n11::n12
+namespace n13 {
+void t();
+}
+namespace n14 {
+void t();
+}
+} // namespace n12
+} // namespace n11
+// CHECK-FIXES: }
+
+namespace n15 {
+namespace n16 {
+void t();
+}
+
+inline namespace n17 { void t(); }
+
+namespace n18 {
+namespace n19 {
+namespace n20 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n18::n19::n20
+void t();
+} // namespace n20
+} // namespace n19
+} // namespace n18
+// CHECK-FIXES: }
+
+namespace n21 {
+void t();
+}
+} // namespace n15
+
+namespace n22 {
+namespace {
+void t();
+}
+} // namespace n22
+
+namespace n23 {
+namespace {
+namespace n24 {
+namespace n25 {
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n24::n25
+void t();
+} // namespace n25
+} // namespace n24
+// CHECK-FIXES: }
+} // namespace
+} // namespace n23
+
+namespace n26::n27 {
+namespace n28 {
+namespace n29::n30 {
+// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: Nested namespaces can be concatenated [modernize-concat-nested-namespaces]
+// CHECK-FIXES: namespace n26::n27::n28::n29::n30
+void t() {}
+} // namespace n29::n30
+} // namespace n28
+} // namespace n26::n27
+// CHECK-FIXES: }
+
+namespace n31 {
+namespace n32 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces are empty and can be removed [modernize-concat-nested-namespaces]
+} // namespace n31
+
+namespace n33 {
+namespace n34 {
+namespace n35 {}
+// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: Nested namespaces are empty and can be removed [modernize-concat-nested-namespaces]
+} // namespace n34
+namespace n36 {
+void t();
+}
+} // namespace n33
+
+namespace n37::n38 {
+void t();
+}
+
+int main() {
+  n26::n27::n28::n29::n30::t();
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/modernize-concat-nested-namespaces.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - modernize-concat-nested-namespaces
+
+modernize-concat-nested-namespaces
+==
+
+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.
+
+For example:
+
+.. code-block:: c++
+  namspace n1 {
+  namespace n2 {
+  void t();
+  }
+  }
+
+  namespace n3 {
+  namespace n4 {
+  namespace n5 {
+  void t();
+  }
+  }
+  namespace n6 {
+  namespace n7 {
+  void t();
+  }
+  }
+  }
+
+Will be modified to:
+
+..