Skip to content

Conversation

evelez7
Copy link
Member

@evelez7 evelez7 commented Oct 10, 2025

Namespace filenames didn't consider their paths, so foo::tools would use
the same file as bar::tools. Now we consider their paths to avoid that
problem.

Copy link
Member Author

evelez7 commented Oct 10, 2025

@evelez7 evelez7 requested review from ilovepi and petrhosek October 10, 2025 17:00
@evelez7 evelez7 marked this pull request as ready for review October 10, 2025 17:00
@llvmbot
Copy link
Member

llvmbot commented Oct 10, 2025

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

Author: Erick Velez (evelez7)

Changes

Namespace filenames didn't consider their paths, so foo::tools would use
the same file as bar::tools. Now we consider their paths to avoid that
problem.


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

3 Files Affected:

  • (modified) clang-tools-extra/clang-doc/JSONGenerator.cpp (+7-1)
  • (added) clang-tools-extra/test/clang-doc/json/multiple-namespaces.cpp (+20)
  • (modified) clang-tools-extra/test/clang-doc/json/nested-namespace.cpp (+1-1)
diff --git a/clang-tools-extra/clang-doc/JSONGenerator.cpp b/clang-tools-extra/clang-doc/JSONGenerator.cpp
index 1b08b1791b6eb..df816d78d77fe 100644
--- a/clang-tools-extra/clang-doc/JSONGenerator.cpp
+++ b/clang-tools-extra/clang-doc/JSONGenerator.cpp
@@ -584,7 +584,13 @@ static SmallString<16> determineFileName(Info *I, SmallString<128> &Path) {
     FileName = RecordSymbolInfo->MangledName;
   } else if (I->USR == GlobalNamespace)
     FileName = "index";
-  else
+  else if (I->IT == InfoType::IT_namespace) {
+    for (const auto &NS : I->Namespace) {
+      FileName += NS.Name;
+      FileName += "_";
+    }
+    FileName += I->Name;
+  } else
     FileName = I->Name;
   sys::path::append(Path, FileName + ".json");
   return FileName;
diff --git a/clang-tools-extra/test/clang-doc/json/multiple-namespaces.cpp b/clang-tools-extra/test/clang-doc/json/multiple-namespaces.cpp
new file mode 100644
index 0000000000000..04fcfc1dc0a85
--- /dev/null
+++ b/clang-tools-extra/test/clang-doc/json/multiple-namespaces.cpp
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: clang-doc --output=%t --format=json --executor=standalone %s
+// RUN: FileCheck %s < %t/json/foo_tools.json --check-prefix=CHECK-FOO
+// RUN: FileCheck %s < %t/json/bar_tools.json --check-prefix=CHECK-BAR
+
+namespace foo {
+  namespace tools {
+    class FooTools {};
+  } // namespace tools
+} // namespace foo
+
+namespace bar {
+  namespace tools {
+    class BarTools {};
+  } // namespace tools
+} // namespace bar
+
+// CHECK-FOO: "Name": "tools"
+
+// CHECK-BAR: "Name": "tools"
diff --git a/clang-tools-extra/test/clang-doc/json/nested-namespace.cpp b/clang-tools-extra/test/clang-doc/json/nested-namespace.cpp
index b19afc1885104..cf19e1e34a818 100644
--- a/clang-tools-extra/test/clang-doc/json/nested-namespace.cpp
+++ b/clang-tools-extra/test/clang-doc/json/nested-namespace.cpp
@@ -1,7 +1,7 @@
 // RUN: rm -rf %t && mkdir -p %t
 // RUN: clang-doc --output=%t --format=json --executor=standalone %s
 // RUN: FileCheck %s < %t/json/nested.json --check-prefix=NESTED
-// RUN: FileCheck %s < %t/json/inner.json --check-prefix=INNER
+// RUN: FileCheck %s < %t/json/nested_inner.json --check-prefix=INNER
 
 namespace nested {
   int Global;

Comment on lines +587 to +593
else if (I->IT == InfoType::IT_namespace) {
for (const auto &NS : I->Namespace) {
FileName += NS.Name;
FileName += "_";
}
FileName += I->Name;
} else
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking we'd be better off just moving away from the the flat dir structure. the number of files/items in a namespace can be quite large, and its probably a better choice to keep the number of files in a directory relatively small. Things like the global namespace may need more thought, since I think they may have lots of things in it. for example C projects will all just have everything in the global namespace, right? so we may also want to consider mirroring the source file directory structure. but moving away from the flat structure first and then using a different schema if we decide its worth the effort seems like a good path.

For now though, I'd like to get these bugs fixed, so lets just add a TODO about that, and go w/ what you have here. I think its more important to have a working thing, than stay in limbo while we figure out a path forward.

@evelez7 evelez7 force-pushed the users/evelez7/clang-doc-fix-index-filename branch from b345c72 to 17048c0 Compare October 10, 2025 19:19
@evelez7 evelez7 force-pushed the users/evelez7/clang-doc-multiple-namespaces branch from 420db87 to 8167825 Compare October 10, 2025 19:19
@evelez7 evelez7 requested a review from ilovepi October 10, 2025 19:19
Base automatically changed from users/evelez7/clang-doc-fix-index-filename to main October 10, 2025 20:34
Namespace filenames didn't consider their paths, so foo::tools would use
the same file as bar::tools. Now we consider their paths to avoid that
problem.
@evelez7 evelez7 force-pushed the users/evelez7/clang-doc-multiple-namespaces branch from 8167825 to 24085f6 Compare October 10, 2025 20:36
Copy link
Member Author

evelez7 commented Oct 11, 2025

Merge activity

  • Oct 11, 10:12 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 11, 10:13 PM UTC: @evelez7 merged this pull request with Graphite.

@evelez7 evelez7 merged commit 220a969 into main Oct 11, 2025
10 checks passed
@evelez7 evelez7 deleted the users/evelez7/clang-doc-multiple-namespaces branch October 11, 2025 22:13
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.

3 participants