Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixing issue #64441 #74814

Conversation

jeevanghimire
Copy link

removing using namespace std;

and assigning the fully qualified name for better naming in codebase

removing using namespace std;

and  assigning the fully qualified name for better naming in codebase
@llvmbot llvmbot added clang Clang issues not falling into any other category lldb labels Dec 8, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 8, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-lldb

Author: Jeevan Ghimire (jeevanghimire)

Changes

removing using namespace std;

and assigning the fully qualified name for better naming in codebase


Full diff: https://github.com/llvm/llvm-project/pull/74814.diff

2 Files Affected:

  • (modified) clang/test/Index/usrs.cpp (+1-1)
  • (modified) lldb/test/API/lang/cpp/namespace/main.cpp (+5-5)
diff --git a/clang/test/Index/usrs.cpp b/clang/test/Index/usrs.cpp
index dbfa44f4b6764..2eeeb82cc5c1f 100644
--- a/clang/test/Index/usrs.cpp
+++ b/clang/test/Index/usrs.cpp
@@ -57,7 +57,7 @@ extern "C" {
 
 namespace foo_alias = foo;
 
-using namespace foo;
+//removing using namespace foo;
 
 namespace foo_alias2 = foo;
 
diff --git a/lldb/test/API/lang/cpp/namespace/main.cpp b/lldb/test/API/lang/cpp/namespace/main.cpp
index 6a8efa160766b..28d5421e1d33d 100644
--- a/lldb/test/API/lang/cpp/namespace/main.cpp
+++ b/lldb/test/API/lang/cpp/namespace/main.cpp
@@ -34,7 +34,7 @@ namespace A {
         int myfunc (int a);
         int myfunc2(int a)
         {
-             return a + 2;
+                return a + 2; //just changing tab not much
         }
         float myfunc (float f)
         {
@@ -56,10 +56,10 @@ namespace Foo = A::B;   // namespace alias
 
 using Foo::myfunc;      // using declaration
 
-using namespace Foo;    // using directive
+//removing namespace foo; for quality naming 
 
-namespace A {
-    namespace B {
+namespace fo::A {
+    namespace foo::B {
         using namespace Y;
         int k;
     }
@@ -140,5 +140,5 @@ main (int argc, char const *argv[])
     ::B::Bar bb;
     A::B::Bar ab;
     return Foo::myfunc(12) + bb.x() + ab.y() + NS1::NS2::Foo{}.bar() +
-           NS2::Foo{}.bar();
+            NS2::Foo{}.bar();
 }

Copy link

github-actions bot commented Dec 8, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 58bdef2be75263a9b6bf93faf3baccc76e31e082 c137cd0ba81f82dbca2feb01bb8d088e42f0c524 -- clang/test/Index/usrs.cpp lldb/test/API/lang/cpp/namespace/main.cpp
View the diff from clang-format here.
diff --git a/lldb/test/API/lang/cpp/namespace/main.cpp b/lldb/test/API/lang/cpp/namespace/main.cpp
index 28d5421e1d..8d45477516 100644
--- a/lldb/test/API/lang/cpp/namespace/main.cpp
+++ b/lldb/test/API/lang/cpp/namespace/main.cpp
@@ -34,7 +34,7 @@ namespace A {
         int myfunc (int a);
         int myfunc2(int a)
         {
-                return a + 2; //just changing tab not much
+          return a + 2; // just changing tab not much
         }
         float myfunc (float f)
         {
@@ -56,14 +56,14 @@ namespace Foo = A::B;   // namespace alias
 
 using Foo::myfunc;      // using declaration
 
-//removing namespace foo; for quality naming 
+// removing namespace foo; for quality naming
 
 namespace fo::A {
-    namespace foo::B {
-        using namespace Y;
-        int k;
-    }
-}
+namespace foo::B {
+using namespace Y;
+int k;
+} // namespace foo::B
+} // namespace fo::A
 
 namespace ns1 {
     int value = 100;
@@ -140,5 +140,5 @@ main (int argc, char const *argv[])
     ::B::Bar bb;
     A::B::Bar ab;
     return Foo::myfunc(12) + bb.x() + ab.y() + NS1::NS2::Foo{}.bar() +
-            NS2::Foo{}.bar();
+           NS2::Foo{}.bar();
 }

@@ -56,10 +56,10 @@ namespace Foo = A::B; // namespace alias

using Foo::myfunc; // using declaration

using namespace Foo; // using directive
//removing namespace foo; for quality naming
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The using directive was used here to make sure LLDB does the right thing when doing qualified lookup in the presence of various DW_TAG_imported_module and name shadowing. I don't think we should change this because it risks subtly changing the test coverage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it can create confusion if we just name the qualified lookup with standard naming it will make more sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jeevanghimire, please note that this is a test, so we must ensure that LLDB does the right thing regardless of the input it receives, even if said input is not considered best practices. The test was likely created to either ensure LLDB works in the case, or because it didn't use to work and someone created a test at the same time the fix was done.

As such, I don't think we should change this.

@@ -34,7 +34,7 @@ namespace A {
int myfunc (int a);
int myfunc2(int a)
{
return a + 2;
return a + 2; //just changing tab not much
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, we don't add diffs that are unrelated to the problem being solved by the PR.
On a similar note, this comment seems to be trying to communicate with reviewers, not future readers of this test file. As such, this is the kind of comment that should be part of the commit message / PR description, and not part of the test itself

Copy link
Contributor

@felipepiovezan felipepiovezan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jeevanghimire, thank you for the PR! I believe the original issue was not about changing LLVM test inputs, but rather about changing clang-tidy to warn about this kind of patterns.
Please see my rationale in a previous comment for why we should not be changing tests.

For future contributions, I'd also recommend using a more descriptive commit title and message. If you inspect the git log of the project, we generally follow the pattern:

[some topic]

For example, this commit message could have been:

[clang-tidy] Implement "using namespace" check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category lldb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants