This is an archive of the discontinued LLVM Phabricator instance.

Differential D97563

[clang-tidy] Enable modernize-concat-nested-namespaces also on headers
ClosedPublic

Authored by DmitryPolukhin on Feb 26 2021, 9:31 AM.

Details

Reviewers
hokein
aaron.ballman
alexfh
JonasToth
njames93
Commits
rGda55af7f1d34: [clang-tidy] Enable modernize-concat-nested-namespaces also on headers
Summary

For some reason the initial implementation of the check had an explicit check
for the main file to avoid being applied in headers. This diff removes this
check and add a test for the check on a header.

Similar approach was proposed in D61989 but review there got stuck.

Test Plan: added new test case

Diff Detail

Event Timeline

DmitryPolukhin created this revision.Feb 26 2021, 9:31 AM
Herald added a subscriber: xazax.hun. · View Herald TranscriptFeb 26 2021, 9:31 AM
DmitryPolukhin requested review of this revision.Feb 26 2021, 9:31 AM
njames93 added inline comments.Feb 26 2021, 9:49 AM
clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h
8

This fix line isn't actually contributing to the test.
Perhaps a better case, assuming clang-format doesn't update the comment, would be:

// CHECK-FIXES: void t();
// CHECK-FIXES-NEXT: } namespace nn1
clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
4

Why do we explicitly run on c++2b and not c++20? I understand there is no framework in place for using -std=c++17-or-later when header files are modified.

DmitryPolukhin updated this revision to Diff 326734.Feb 26 2021, 10:15 AM

Comments resolved

DmitryPolukhin marked an inline comment as done.Feb 26 2021, 10:18 AM

@njames93 thank you for quick response and good suggestion!

clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
4

There is no good reason, I just took highest current version. Changed to c++20.

DmitryPolukhin marked an inline comment as done.Feb 26 2021, 10:18 AM
njames93 added inline comments.Feb 26 2021, 11:08 AM
clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp
4

This is just me rambling, but i think we should think about introducing a test mode to clang tidy, where fixes can be applied to a test directory and maybe try and make use of the verify diagnostic consumer. The current implementation works, but is very rough round the edges.

Harbormaster completed remote builds in B91061: Diff 326724.Feb 26 2021, 12:08 PM
Harbormaster completed remote builds in B91070: Diff 326734.Feb 26 2021, 1:28 PM
DmitryPolukhin updated this revision to Diff 326888.Feb 27 2021, 3:53 AM

Use -DAG checks to make test stable

Harbormaster completed remote builds in B91179: Diff 326888.Feb 27 2021, 4:25 AM
DmitryPolukhin added a comment.Mar 9 2021, 1:16 AM

@aaron.ballman @alexfh @njames93 - friendly ping, please take a look!

njames93 accepted this revision.Mar 14 2021, 5:12 AM

Is DAG required because the header file warnings are printed in a different order depending on things like platform?

This revision is now accepted and ready to land.Mar 14 2021, 5:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 14 2021, 5:12 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
DmitryPolukhin updated this revision to Diff 330578.Mar 15 2021, 2:01 AM

Rebase

lebedev.ri added a subscriber: lebedev.ri.Mar 15 2021, 2:04 AM

I think the implicit question is: won't this regress headers that are meant to be compatible with earlier standards?
Did the original review mention anything about this?

Harbormaster completed remote builds in B93757: Diff 330578.Mar 15 2021, 2:33 AM
njames93 added a comment.Mar 15 2021, 3:12 AM
In D97563#2625513, @lebedev.ri wrote:

I think the implicit question is: won't this regress headers that are meant to be compatible with earlier standards?
Did the original review mention anything about this?

Clang-tidy already deals with that using the --header-filter option.

DmitryPolukhin added a comment.Mar 15 2021, 3:22 AM
In D97563#2624680, @njames93 wrote:

Is DAG required because the header file warnings are printed in a different order depending on things like platform?

@njames93 Thank you for review! Yes, on Windows build bot showed different order of the messages so I added -DAG to don't depend on any particular order.

In D97563#2625513, @lebedev.ri wrote:

I think the implicit question is: won't this regress headers that are meant to be compatible with earlier standards?
Did the original review mention anything about this?

In the original code review it was not discussed. Moreover it is more or less applicable to all modernize-* checks and they work in headers selected with --header-filter so I think these is no reason to invent special mechanism only for this check.

DmitryPolukhin updated this revision to Diff 330596.Mar 15 2021, 3:41 AM

Rebase again

Harbormaster completed remote builds in B93771: Diff 330596.Mar 15 2021, 4:27 AM
Closed by commit rGda55af7f1d34: [clang-tidy] Enable modernize-concat-nested-namespaces also on headers (authored by DmitryPolukhin). · Explain WhyMar 15 2021, 7:37 AM
This revision was automatically updated to reflect the committed changes.
DmitryPolukhin added a commit: rGda55af7f1d34: [clang-tidy] Enable modernize-concat-nested-namespaces also on headers.

Revision Contents

PathSize
clang-tools-extra/
clang-tidy/
modernize/
3 lines
test/
clang-tidy/
checkers/
Inputs/
modernize-concat-nested-namespaces/
8 lines
29 lines

Diff 326888

clang-tools-extra/clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp

Show First 20 Lines • Show All 76 Lines • ▼ Show 20 Lines
void ConcatNestedNamespacesCheck::check( void ConcatNestedNamespacesCheck::check(
const ast_matchers::MatchFinder::MatchResult &Result) { const ast_matchers::MatchFinder::MatchResult &Result) {
const NamespaceDecl &ND = *Result.Nodes.getNodeAs<NamespaceDecl>("namespace"); const NamespaceDecl &ND = *Result.Nodes.getNodeAs<NamespaceDecl>("namespace");
const SourceManager &Sources = *Result.SourceManager; const SourceManager &Sources = *Result.SourceManager;
if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc())) if (!locationsInSameFile(Sources, ND.getBeginLoc(), ND.getRBraceLoc()))
return; return;
if (!Sources.isInMainFile(ND.getBeginLoc()))
return;
if (anonymousOrInlineNamespace(ND)) if (anonymousOrInlineNamespace(ND))
return; return;
Namespaces.push_back(&ND); Namespaces.push_back(&ND);
if (singleNamedNamespaceChild(ND)) if (singleNamedNamespaceChild(ND))
return; return;
Show All 15 Lines

clang-tools-extra/test/clang-tidy/checkers/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h

  • This file was added.
namespace nn1 {
namespace nn2 {
// CHECK-FIXES: namespace nn1::nn2
void t();
} // namespace nn2
} // namespace nn1
// CHECK-FIXES: void t();
// CHECK-FIXES-NEXT: } // namespace nn1
njames93Unsubmitted
Done
ReplyInline Actions

This fix line isn't actually contributing to the test.
Perhaps a better case, assuming clang-format doesn't update the comment, would be:

// CHECK-FIXES: void t();
// CHECK-FIXES-NEXT: } namespace nn1
njames93: This fix line isn't actually contributing to the test. Perhaps a better case, assuming clang…

clang-tools-extra/test/clang-tidy/checkers/modernize-concat-nested-namespaces.cpp

// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-concat-nested-namespaces %t // RUN: cp %S/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h %T/modernize-concat-nested-namespaces.h
// RUN: %check_clang_tidy -std=c++17 %s modernize-concat-nested-namespaces %t -- -header-filter=".*" -- -I %T
// RUN: FileCheck -input-file=%T/modernize-concat-nested-namespaces.h %S/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h -check-prefix=CHECK-FIXES
// Restore header file and re-run with c++20:
njames93Unsubmitted
Done
ReplyInline Actions

Why do we explicitly run on c++2b and not c++20? I understand there is no framework in place for using -std=c++17-or-later when header files are modified.

njames93: Why do we explicitly run on c++2b and not c++20? I understand there is no framework in place…
DmitryPolukhinAuthorUnsubmitted
Done
ReplyInline Actions

There is no good reason, I just took highest current version. Changed to c++20.

DmitryPolukhin: There is no good reason, I just took highest current version. Changed to c++20.
njames93Unsubmitted
Done
ReplyInline Actions

This is just me rambling, but i think we should think about introducing a test mode to clang tidy, where fixes can be applied to a test directory and maybe try and make use of the verify diagnostic consumer. The current implementation works, but is very rough round the edges.

njames93: This is just me rambling, but i think we should think about introducing a test mode to clang…
// RUN: cp %S/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h %T/modernize-concat-nested-namespaces.h
// RUN: %check_clang_tidy -std=c++20 %s modernize-concat-nested-namespaces %t -- -header-filter=".*" -- -I %T
// RUN: FileCheck -input-file=%T/modernize-concat-nested-namespaces.h %S/Inputs/modernize-concat-nested-namespaces/modernize-concat-nested-namespaces.h -check-prefix=CHECK-FIXES
#include "modernize-concat-nested-namespaces.h"
// CHECK-MESSAGES-DAG: modernize-concat-nested-namespaces.h:1:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
namespace n1 {} namespace n1 {}
namespace n2 { namespace n2 {
namespace n3 { namespace n3 {
void t(); void t();
} }
namespace n4 { namespace n4 {
Show All 12 Lines
namespace n8 { namespace n8 {
void t(); void t();
} }
} // namespace n7 } // namespace n7
namespace n9 { namespace n9 {
namespace n10 { namespace n10 {
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n9::n10 // CHECK-FIXES: namespace n9::n10
void t(); void t();
} // namespace n10 } // namespace n10
} // namespace n9 } // namespace n9
// CHECK-FIXES: } // CHECK-FIXES: }
namespace n11 { namespace n11 {
namespace n12 { namespace n12 {
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n11::n12 // CHECK-FIXES: namespace n11::n12
namespace n13 { namespace n13 {
void t(); void t();
} }
namespace n14 { namespace n14 {
void t(); void t();
} }
} // namespace n12 } // namespace n12
} // namespace n11 } // namespace n11
// CHECK-FIXES: } // CHECK-FIXES: }
namespace n15 { namespace n15 {
namespace n16 { namespace n16 {
void t(); void t();
} }
inline namespace n17 { inline namespace n17 {
void t(); void t();
} }
namespace n18 { namespace n18 {
namespace n19 { namespace n19 {
namespace n20 { namespace n20 {
// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n18::n19::n20 // CHECK-FIXES: namespace n18::n19::n20
void t(); void t();
} // namespace n20 } // namespace n20
} // namespace n19 } // namespace n19
} // namespace n18 } // namespace n18
// CHECK-FIXES: } // CHECK-FIXES: }
namespace n21 { namespace n21 {
void t(); void t();
} }
} // namespace n15 } // namespace n15
namespace n22 { namespace n22 {
namespace { namespace {
void t(); void t();
} }
} // namespace n22 } // namespace n22
namespace n23 { namespace n23 {
namespace { namespace {
namespace n24 { namespace n24 {
namespace n25 { namespace n25 {
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n24::n25 // CHECK-FIXES: namespace n24::n25
void t(); void t();
} // namespace n25 } // namespace n25
} // namespace n24 } // namespace n24
// CHECK-FIXES: } // CHECK-FIXES: }
} // namespace } // namespace
} // namespace n23 } // namespace n23
namespace n26::n27 { namespace n26::n27 {
namespace n28 { namespace n28 {
namespace n29::n30 { namespace n29::n30 {
// CHECK-MESSAGES: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-3]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n26::n27::n28::n29::n30 // CHECK-FIXES: namespace n26::n27::n28::n29::n30
void t() {} void t() {}
} // namespace n29::n30 } // namespace n29::n30
} // namespace n28 } // namespace n28
} // namespace n26::n27 } // namespace n26::n27
// CHECK-FIXES: } // CHECK-FIXES: }
namespace n31 { namespace n31 {
namespace n32 {} namespace n32 {}
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
} // namespace n31 } // namespace n31
// CHECK-FIXES-EMPTY // CHECK-FIXES-EMPTY
namespace n33 { namespace n33 {
namespace n34 { namespace n34 {
namespace n35 {} namespace n35 {}
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
} // namespace n34 } // namespace n34
// CHECK-FIXES-EMPTY // CHECK-FIXES-EMPTY
namespace n36 { namespace n36 {
void t(); void t();
} }
} // namespace n33 } // namespace n33
namespace n37::n38 { namespace n37::n38 {
void t(); void t();
} }
#define IEXIST #define IEXIST
namespace n39 { namespace n39 {
namespace n40 { namespace n40 {
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n39::n40 // CHECK-FIXES: namespace n39::n40
#ifdef IEXIST #ifdef IEXIST
void t() {} void t() {}
#endif #endif
} // namespace n40 } // namespace n40
} // namespace n39 } // namespace n39
// CHECK-FIXES: } // CHECK-FIXES: }
namespace n41 { namespace n41 {
namespace n42 { namespace n42 {
// CHECK-MESSAGES: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces] // CHECK-MESSAGES-DAG: :[[@LINE-2]]:1: warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]
// CHECK-FIXES: namespace n41::n42 // CHECK-FIXES: namespace n41::n42
#ifdef IDONTEXIST #ifdef IDONTEXIST
void t() {} void t() {}
#endif #endif
} // namespace n42 } // namespace n42
} // namespace n41 } // namespace n41
// CHECK-FIXES: } // CHECK-FIXES: }
Show All 12 Lines