Skip to content

Conversation

@kwk
Copy link
Contributor

@kwk kwk commented Jul 11, 2025

This adds rst documentation for the clang-change-namespace program.

I ran the examples locally to ensure they work.

Fixes #35519

@kwk kwk requested a review from LegalizeAdulthood July 11, 2025 18:46
@kwk kwk self-assigned this Jul 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 11, 2025

@llvm/pr-subscribers-clang-tools-extra

Author: Konrad Kleine (kwk)

Changes

This adds rst documentation for the clang-change-namespace program.

I ran the examples locally to ensure they work.

See #35519


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

2 Files Affected:

  • (added) clang-tools-extra/docs/clang-change-namespace.rst (+158)
  • (modified) clang-tools-extra/docs/index.rst (+1)
diff --git a/clang-tools-extra/docs/clang-change-namespace.rst b/clang-tools-extra/docs/clang-change-namespace.rst
new file mode 100644
index 0000000000000..23c3dcc8b7412
--- /dev/null
+++ b/clang-tools-extra/docs/clang-change-namespace.rst
@@ -0,0 +1,158 @@
+======================
+Clang-Change-Namespace
+======================
+
+.. contents::
+
+.. toctree::
+  :maxdepth: 1
+
+:program:`clang-change-namespace` can be used to change the surrounding
+namespaces of class/function definitions.
+
+Classes/functions in the moved namespace will have new namespaces while
+references to symbols (e.g. types, functions) which are not defined in the
+changed namespace will be correctly qualified by prepending namespace specifiers
+before them. This will try to add shortest namespace specifiers possible.
+
+When a symbol reference needs to be fully-qualified, this adds a `::` prefix to
+the namespace specifiers unless the new namespace is the global namespace. For
+classes, only classes that are declared/defined in the given namespace in
+specified files will be moved: forward declarations will remain in the old
+namespace. The will be demonstrated in the next example.
+
+Example usage
+-------------
+
+For example, consider this `test.cc` example here with the forward declared
+class `FWD` and the defined class `A`, both in the namespace `a`.
+
+.. code-block:: c++
+  :linenos:
+
+  namespace a {
+  class FWD;
+  class A {
+    FWD *fwd;
+  };
+  } // namespace a
+
+And now let's change the namespace `a` to `x`.
+
+.. code-block:: console
+  clang-change-namespace \
+    --old_namespace "a" \
+    --new_namespace "x" \
+    --file_pattern "test.cc" \
+    --i \
+    test.cc
+
+Note that in the code below there's still the forward decalred class `FWD` that
+stayed in the namespace `a`. It wasn't moved to the new namespace because it
+wasn't defined/declared here in `a` but only forward declared.
+
+.. code-block:: c++
+  :linenos:
+
+  namespace a {
+  class FWD;
+  } // namespace a
+  namespace x {
+
+  class A {
+    a::FWD *fwd;
+  };
+  } // namespace x
+
+
+Another example
+---------------
+
+Consider this `test.cc` file:
+
+.. code-block:: c++
+  :linenos:
+
+  namespace na {
+  class X {};
+  namespace nb {
+  class Y {
+    X x;
+  };
+  } // namespace nb
+  } // namespace na
+
+To move the definition of class `Y` from namespace `na::nb` to `x::y`, run:
+
+.. code-block:: console
+
+  clang-change-namespace \
+    --old_namespace "na::nb" \
+    --new_namespace "x::y" \
+    --file_pattern "test.cc" \
+    --i \
+    test.cc
+
+This will overwrite `test.cc` to look like this:
+
+.. code-block:: c++
+  :linenos:
+
+  namespace na {
+  class X {};
+
+  } // namespace na
+  namespace x {
+  namespace y {
+  class Y {
+    na::X x;
+  };
+  } // namespace y
+  } // namespace x
+
+Note, that we've successfully moved the class `Y` from namespace `na::nb` to
+namespace `x::y`.
+
+:program:`clang-change-namespace` Command Line Options
+======================================================
+
+.. option:: --allowed_file=<string>     
+
+  A file containing regexes of symbol names that are not expected to be updated
+  when changing namespaces around them.
+
+.. option:: --dump_result               
+
+  Dump new file contents in YAML, if specified.
+
+.. option:: --extra-arg=<string>        
+
+  Additional argument to append to the compiler command line
+
+.. option:: --extra-arg-before=<string> 
+
+  Additional argument to prepend to the compiler command line
+
+.. option:: --file_pattern=<string>     
+
+  Only rename namespaces in files that match the given pattern.
+
+.. option:: -i                          
+ 
+  Inplace edit <file>s, if specified.
+
+.. option:: --new_namespace=<string>    
+ 
+  New namespace.
+
+.. option:: --old_namespace=<string>    
+ 
+  Old namespace.
+
+.. option:: -p <string>                 
+ 
+  Build path
+
+.. option:: --style=<string>            
+ 
+  The style name used for reformatting.
diff --git a/clang-tools-extra/docs/index.rst b/clang-tools-extra/docs/index.rst
index 9f7324fcf7419..3f3a99d1b70c6 100644
--- a/clang-tools-extra/docs/index.rst
+++ b/clang-tools-extra/docs/index.rst
@@ -17,6 +17,7 @@ Contents
 
    clang-tidy/index
    clang-include-fixer
+   clang-change-namespace
    modularize
    pp-trace
    clangd <https://clangd.llvm.org/>

@kwk kwk requested review from AaronBallman and tJener July 12, 2025 08:02
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for working on the documentation for this, it's really appreciated! I had a few questions, but in general this is looking good.

Comment on lines +18 to +19
Copy link
Collaborator

@AaronBallman AaronBallman Jul 18, 2025

Choose a reason for hiding this comment

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

I'm not certain I understand this. Is this saying:

$ cat > test.cc << EOF
  namespace a {
  class A {
  };    
  } // namespace a

  a::A thing;
EOF

with

  clang-change-namespace \
    --old_namespace "a" \
    --new_namespace "" \
    --file_pattern "test.cc" \
    --i \
    test.cc

will produce A thing; but

  clang-change-namespace \
    --old_namespace "a" \
    --new_namespace "foo" \
    --file_pattern "test.cc" \
    --i \
    test.cc

will produce ::foo::A thing;?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understood this as fully-qualifying the namespace when necessary to refer to the correct namespace. For example,

namespace other::foo {
a::A thing;
}  // namespace other::foo
namespace foo {
a::A thing;
}  // namespace foo
namespace other {
a::A thing;
struct foo {
  a::A thing;
};
a::A thing2;
}  // namespace other

becomes

namespace other::foo {
::foo::A thing;
}  // namespace other::foo
namespace foo {
A thing;
}  // namespace foo
namespace other {
foo::A thing;
struct foo {
  ::foo::A thing;
};
::foo:A thing2;
}  // namespace other

@kwk is that understanding correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried running the binary and after looking at the tests, I believe I've misunderstood. It looks like the tool only changes symbol references within the moved namespace. It does not update any symbol references from outside the moved namespace that point to symbols inside the moved namespace (which would no longer reference a valid symbol after the changes are applied).

Concrete example:

// a.cc
namespace old {
struct foo {};
}  // namespace old

namespace b {
old::foo g_foo;
}  // namespace b
$ clang-change-namespace --old_namespace 'old' --new_namespace 'new' --file_pattern '.*' a.cc
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "a.cc"
No compilation database found in /tmp or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
================= a.cc ================


namespace new {
  struct foo {};
} // namespace new

namespace b {
old::foo g_foo;
}  // namespace b

============================================

I think that is worth clarifying that this only updates symbol references in the moved namespace. (Unfortunately, I feel like that makes this tool much less useful.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not documented in db0021421e6b.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps not a question on the documentation, but rather the tool. Why is the global namespace special in this regard? It seems like there could be name ambiguities that arise with moving to the global namespace.

Copy link
Contributor Author

@kwk kwk Jul 19, 2025

Choose a reason for hiding this comment

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

@tJener that is correct. As @nikic pointed out in a chat, this tool looks unmaintained. Most of the content changes in here look like as if they are 9 years old. The fact that you, @tJener and @AaronBallman ask these very specific questions lets me think that it is also not in active use. I'm going to go on PTO soon but I will put all of your questions on my list to address when I'm back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I suspect you may be looking for an Eric Liu who appears to have written the majority of the tool, which is not me. Which is to say that my lack of familiarity with this tool shouldn't really mean much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps not a question on the documentation, but rather the tool. Why is the global namespace special in this regard? It seems like there could be name ambiguities that arise with moving to the global namespace.

Consider this example to showcase ambiguities in names being used more than once:

cat test.cc 
namespace a {
class A {
  int classAFromWithinNamespace_a;
};
} // namespace a

class A {
  int classAInGlobalNamespace;
};

Then run clang-change-namespace --old_namespace "a" --new_namespace "" --file_pattern test.cc test.cc and the result will look like this:

class A {
  int classAFromWithinNamespace_a;
};

class A {
  int classAInGlobalNamespace;
};

The re-factoring looks correct but the code will not compile due to the name duplication. I'd say it is not up to the tool to ensure compilability in that sense. Of course, if you agree to this, then this MUST be documented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tJener that is correct. As @nikic pointed out in a chat, this tool looks unmaintained. Most of the content changes in here look like as if they are 9 years old. The fact that you, @tJener and @AaronBallman ask these very specific questions lets me think that it is also not in active use. I'm going to go on PTO soon but I will put all of your questions on my list to address when I'm back.

Yeah, the code is basically unmaintained at this point, though I suspect that's because it's in "good enough" state that it doesn't require much active work. I'd say let's document the state of things as they are.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say it is not up to the tool to ensure compilability in that sense. Of course, if you agree to this, then this MUST be documented.

I think that's worth documenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've documented "Content already exists in new namespace" in 73a64b462271.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be worth specifying that this is a regular expression, as opposed to some other syntax (e.g. glob).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The regular expression parameter is now documented in 5ca20a39227c

Comment on lines +18 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried running the binary and after looking at the tests, I believe I've misunderstood. It looks like the tool only changes symbol references within the moved namespace. It does not update any symbol references from outside the moved namespace that point to symbols inside the moved namespace (which would no longer reference a valid symbol after the changes are applied).

Concrete example:

// a.cc
namespace old {
struct foo {};
}  // namespace old

namespace b {
old::foo g_foo;
}  // namespace b
$ clang-change-namespace --old_namespace 'old' --new_namespace 'new' --file_pattern '.*' a.cc
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "a.cc"
No compilation database found in /tmp or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
================= a.cc ================


namespace new {
  struct foo {};
} // namespace new

namespace b {
old::foo g_foo;
}  // namespace b

============================================

I think that is worth clarifying that this only updates symbol references in the moved namespace. (Unfortunately, I feel like that makes this tool much less useful.)

@kwk
Copy link
Contributor Author

kwk commented Aug 18, 2025

@AaronBallman @tJener I've addressed all of you comments and hope this PR is now good to merge. Could you give it one last review? Am I supposed to mark the discussions we had as Resolved or is that up to you?

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

The new docs look awesome to me, thank you!

@AaronBallman
Copy link
Collaborator

@AaronBallman @tJener I've addressed all of you comments and hope this PR is now good to merge. Could you give it one last review? Am I supposed to mark the discussions we had as Resolved or is that up to you?

I gave it a review and my LG, but let's wait a bit for @tJener to weigh in before landing.

As for marking discussions resolved, I usually let the person who left the original comment decide if it's resolved or not, but others mark things resolved when they think they've addressed all the feedback, so it's not consistent. :-)

Copy link
Contributor

@tJener tJener left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this meant to move namespace a to namespace b, and not the global namespace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. I wrote this and then jumped to adding the documentation to the parameter instead. Fixed in 30bf1f8.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the top or bottom, you can summarize the caveat behavior as only symbol references in the moved namespace are updated, references outside of it will not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 55e69ff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: I don't like the use of "Apparently" in documentation, as it makes it seem almost a surprise, which feels wrong for documentation. Up to you though, as this is more of a style nit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get that and you're right. Removed in 64ad789.

Copy link
Contributor

Choose a reason for hiding this comment

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

Optional nit: The previous command lines in the above sections broke up the arguments on separate lines. Not sure of the difference here is intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I went with consistency and modified the oneliner by breaking it up in c653ba2.

@kwk kwk force-pushed the fix-35519-clang-change-namespace branch from a581f0f to c653ba2 Compare August 18, 2025 16:05
@kwk
Copy link
Contributor Author

kwk commented Aug 18, 2025

@tJener I'm sorry I've messed up. I rebased my changes onto main and force pushed them here. If you could give it one last spin, that would be great, then I can squash and merge.

@tJener
Copy link
Contributor

tJener commented Aug 18, 2025

I'm sorry I've messed up.

No need for the apology; code review is there to help improve the final outcome.

@kwk kwk merged commit f5a648f into llvm:main Aug 18, 2025
10 checks passed
@kwk kwk deleted the fix-35519-clang-change-namespace branch August 18, 2025 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clang-change-namespace tool is not documented

4 participants