create new clang-tidy check to add namespaces to symbol references #70621

daltairwalter · 2023-10-30T05:46:35Z

What should be done when "using namespace" has been abused or used in header files? This check will update all references to include full namespace qualification.

Closes #64441

github-actions · 2023-10-30T05:49:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

EugeneZelenko · 2023-10-30T14:53:38Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+}
+
+std::string getIdentifierString(const IdentifierInfo *identifier) {
+  return (identifier) ? identifier->getNameStart() : "(nullptr)";


Are brackets around identifier necessary?

EugeneZelenko · 2023-10-30T14:54:22Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+      limitToPattern(Options.get("LimitToPattern", "")),
+      onlyExpandUsingNamespace(Options.get("OnlyExpandUsingNamespace", true)),
+      diagnosticLevel(Options.get("DiagnosticLevel", 0)) {
+  auto limitString = limitToPattern.str();


Please do not use auto unless type is explicitly spelled in same statement or iterator. Same in other places.

EugeneZelenko · 2023-10-30T14:58:31Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+  for (; currentTargetIndex < targetContextVector.size();
+       ++currentTargetIndex) {
+    auto currentContext = targetContextVector[currentTargetIndex];
+    if (auto currentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {


const auto *. Same in other places.

EugeneZelenko · 2023-10-30T15:00:27Z

clang-tools-extra/docs/clang-tidy/checks/readability/use-explicit-namespaces.rst

+readability-use-explicit-namespaces
+===================================
+
+This check detects and fixes references to members of namespaces where the namespace is not explicity specified in the reference.  By default, this will only change references that are found through a using namespace directive.


Please synchronize first statement with Release Notes. This check is omitted. Please follow 80 characters limit. Double space before This check.

EugeneZelenko · 2023-10-30T15:01:25Z

clang-tools-extra/docs/clang-tidy/checks/readability/use-explicit-namespaces.rst

+
+This check detects and fixes references to members of namespaces where the namespace is not explicity specified in the reference.  By default, this will only change references that are found through a using namespace directive.
+
+Example:


See other documentation for code block examples.

EugeneZelenko · 2023-10-30T15:01:36Z

clang-tools-extra/docs/clang-tidy/checks/readability/use-explicit-namespaces.rst

+string something = "something";    // this is not change by default because it is referenced through using std::string instead of using namespace std
+std::cout << something;
+
+Options:


See other documentation for options examples.

EugeneZelenko · 2023-10-30T15:01:58Z

clang-tools-extra/test/clang-tidy/checkers/readability/use-explicit-namespaces.cpp

+	// CHECK-MESSAGES: :[[@LINE+1]]:2: warning: Missing namespace qualifiers foo::
+        doSomething();
+}
+


Excessive newline.

PiotrZSL · 2023-10-30T15:35:49Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+}
+
+std::string getIdentifierString(const IdentifierInfo *identifier) {
+  return (identifier) ? identifier->getNameStart() : "(nullptr)";


Use getName() and return StringRef instead of std::string

PiotrZSL · 2023-10-30T15:36:15Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+  return (identifier) ? identifier->getNameStart() : "(nullptr)";
+}
+
+std::string getMatchContext(const char *match, const DynTypedNode &node) {


use StringRef instead of "const char*"

PiotrZSL · 2023-10-30T15:36:37Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+  return out.str();
+}
+
+std::string trueFalseString(bool value) { return value ? "true" : "false"; }


return StringRef

PiotrZSL · 2023-10-30T15:37:17Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+
+std::string getMatchContext(const char *match, const DynTypedNode &node) {
+  std::stringstream out;
+  out << match << "(" << node.getNodeKind().asStringRef().str() << ")";


Avoid std::stringstream, it's expensive to create, just merge those strings, you may use SmallString

PiotrZSL · 2023-10-30T15:38:03Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+UseExplicitNamespacesCheck::UseExplicitNamespacesCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      limitToPattern(Options.get("LimitToPattern", "")),


General: Variables and members should start with Upcase.

PiotrZSL · 2023-10-30T15:40:32Z

clang-tools-extra/test/clang-tidy/checkers/readability/use-explicit-namespaces.cpp

@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s readability-use-explicit-namespaces %t
+


Add tests with:

templates

operators

classes

fixes

...

Are there tools or resources for this? I locally have done a lot more testing, but it is very clear how to turn this into test cases.

PiotrZSL · 2023-10-30T15:40:57Z

clang-tools-extra/docs/clang-tidy/checks/readability/use-explicit-namespaces.rst

+Options:
+LimitToPattern - only change things that match pattern e.g. "value: std" would limit changes to the std namespace
+OnlyExpandUsingNamespace - defaults to true meaning that only references found by using namespace are changed
+DiagnosticLevel - add diagnostic information about the choices being made by this check and what it is internally using


This option may not be needed or should be described better,

I think it may make sense to remove DiagnosticLevel if clang-tidy has a similar concept already - could you identify this?

PiotrZSL · 2023-10-30T15:41:42Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.h

+
+  IdentifierInfo *
+  getTypeNestedNameIdentfierRecursive(const Type *type,
+                                      std::string &nestedTypeInfo);
+  bool matchesNamespaceLimits(
+      const std::vector<const DeclContext *> &targetContextVector);
+
+  void diagOut(const SourceLocation &sourcePosition,
+               const std::string &message);
+
+  void processTransform(NestedNameSpecifierLoc nestedName,
+                        const SourceLocation &sourcePosition,
+                        const NamedDecl *target,
+                        const DeclContext *referenceContext, bool usingShadow,
+                        const std::string &context);
+
+  void processTypePiecesRecursive(NestedNameSpecifierLoc nestedName,
+                                  const TypeLoc &typeLoc,
+                                  const DeclContext *declContext,
+                                  const std::string &context);
+
+  void processTypePieces(TypeSourceInfo *typeSourceInfo,
+                         const DeclContext *declContext,
+                         const std::string &context);


Make all those methods private, and if they not needed make them non-class members private methods in .cpp

PiotrZSL · 2023-10-30T15:42:25Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+        if (currentNamespace->isAnonymousNamespace()) {
+          return false;
+        }
+        if (currentNamespace->getIdentifier()->getName().str() == name) {


this may crash, getIdentifier may return nullptr;

PiotrZSL · 2023-10-30T15:42:45Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+    return true;
+  }
+  size_t currentTargetIndex = 0;
+  auto findMatch = [&currentTargetIndex,


extract this lambda to separate function

PiotrZSL · 2023-10-30T15:43:11Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+  size_t currentTargetIndex = 0;
+  auto findMatch = [&currentTargetIndex,
+                    &targetContextVector](const std::string &name) -> bool {
+    while (currentTargetIndex < targetContextVector.size()) {


use iterators.

PiotrZSL · 2023-10-30T15:43:49Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+    while (currentTargetIndex < targetContextVector.size()) {
+      auto currentContext = targetContextVector[currentTargetIndex];
+      ++currentTargetIndex;
+      if (auto currentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {


Suggested change

if (auto currentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {

if (const auto* CurrentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {

PiotrZSL · 2023-10-30T15:44:44Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+        if (!currentNamespace->isInline()) {
+          return false;
+        }
+      } else {


Reduce amount of code blocks, and things like if/else/return

PiotrZSL · 2023-10-30T15:44:59Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+    }
+    return false;
+  };
+  for (size_t i = 0; i < limitToPatternVector.size(); ++i) {


use iterators or range-for

PiotrZSL · 2023-10-30T15:45:39Z

clang-tools-extra/clang-tidy/readability/UseExplicitNamespacesCheck.cpp

+  return true;
+}
+
+IdentifierInfo *UseExplicitNamespacesCheck::getTypeNestedNameIdentfierRecursive(


Not needed to make check working, remove custom diagnostic.

PiotrZSL

I like an idea, but this check require lot of work.
Todo:

Update to LLVM codding standard - https://llvm.org/docs/CodingStandards.html
Remove any custom diagnostic and things (noise) that aren't needed to make check function
Update code to C++17
Add more test
Improve documentation

daltairwalter added 3 commits October 30, 2023 00:08

Initial commit of add new check before changes

f44d774

Initial commit of changes for explicit namespace check changes

5af593d

add test and release notes

38df6a9

llvmbot added the clang-tidy label Oct 30, 2023

update formatting for newer version of clang-format

f86d9e1

EugeneZelenko requested changes Oct 30, 2023

View reviewed changes

EugeneZelenko requested review from PiotrZSL and carlosgalvezp October 30, 2023 15:02

PiotrZSL reviewed Oct 30, 2023

View reviewed changes

PiotrZSL requested changes Oct 30, 2023

View reviewed changes

fix some review issues

912884d

PiotrZSL mentioned this pull request Dec 10, 2023

clang-tidy: fully qualify names from a given namespace (e.g. clean up using namespace std;) #64441

Open

daltairwalter mentioned this pull request Dec 26, 2023

clang-tidy: provide a method for checks to produce self-diagnostic messages #76399

Open

daltairwalter and others added 2 commits January 2, 2024 22:16

Merge branch 'llvm:main' into main

72629d5

cleanup test case

cb28f74

daltairwalter added 3 commits January 4, 2024 22:43

add more test cases

3329732

stop using lambda for findMatch

9a0684f

add more examples of what not to qualify

160bd8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create new clang-tidy check to add namespaces to symbol references #70621

create new clang-tidy check to add namespaces to symbol references #70621

daltairwalter commented Oct 30, 2023 •

edited by PiotrZSL

github-actions bot commented Oct 30, 2023 •

edited

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

EugeneZelenko Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

daltairwalter Oct 31, 2023

PiotrZSL Oct 30, 2023

daltairwalter Oct 31, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL Oct 30, 2023

PiotrZSL left a comment


		This check detects and fixes references to members of namespaces where the namespace is not explicity specified in the reference. By default, this will only change references that are found through a using namespace directive.

		Example:

		@@ -0,0 +1,22 @@
		// RUN: %check_clang_tidy %s readability-use-explicit-namespaces %t

	if (auto currentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {
	if (const auto* CurrentNamespace = dyn_cast<NamespaceDecl>(currentContext)) {

create new clang-tidy check to add namespaces to symbol references #70621

Are you sure you want to change the base?

create new clang-tidy check to add namespaces to symbol references #70621

Conversation

daltairwalter commented Oct 30, 2023 • edited by PiotrZSL

github-actions bot commented Oct 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PiotrZSL left a comment

Choose a reason for hiding this comment

daltairwalter commented Oct 30, 2023 •

edited by PiotrZSL

github-actions bot commented Oct 30, 2023 •

edited