-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang-doc] lower filename length limit by 5 #155511
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
Conversation
@llvm/pr-subscribers-clang-tools-extra Author: Erick Velez (evelez7) ChangesThe previous limit did not take into account filename extensions. Since Also add some tests to make sure the rules are observed and don't Full diff: https://github.com/llvm/llvm-project/pull/155511.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-doc/Serialize.cpp b/clang-tools-extra/clang-doc/Serialize.cpp
index bcab4f1b8a729..fdcf3b29fb754 100644
--- a/clang-tools-extra/clang-doc/Serialize.cpp
+++ b/clang-tools-extra/clang-doc/Serialize.cpp
@@ -778,7 +778,7 @@ static void populateSymbolInfo(SymbolInfo &I, const T *D, const FullComment *C,
Mangler->mangleCXXVTable(CXXD, MangledStream);
else
MangledStream << D->getNameAsString();
- if (MangledName.size() > 255)
+ if (MangledName.size() > 250)
// File creation fails if the mangled name is too long, so default to the
// USR. We should look for a better check since filesystems differ in
// maximum filename length
diff --git a/clang-tools-extra/test/clang-doc/long-name.cpp b/clang-tools-extra/test/clang-doc/long-name.cpp
new file mode 100644
index 0000000000000..b33337588da19
--- /dev/null
+++ b/clang-tools-extra/test/clang-doc/long-name.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t && mkdir -p %t
+// RUN: clang-doc --output=%t --format=mustache --executor=standalone %s
+// RUN: ls %t/json | FileCheck %s -check-prefix=CHECK-JSON
+// RUN: ls %t/html | FileCheck %s -check-prefix=CHECK-HTML
+
+struct ThisStructHasANameThatResultsInAMangledNameThatIsExactly250CharactersLongThatIsSupposedToTestTheFilenameLengthLimitsWithinClangDocInOrdertoSeeifclangdocwillcrashornotdependingonthelengthofthestructIfTheLengthIsTooLongThenClangDocWillCrashAnd12 {};
+
+// This name is 1 character over the limit, so it will be serialized as a USR.
+struct ThisStructHasANameThatResultsInAMangledNameThatIsExactly251CharactersLongThatIsSupposedToTestTheFilenameLengthLimitsWithinClangDocInOrdertoSeeifclangdocwillcrashornotdependingonthelengthofthestructIfTheLengthIsTooLongThenClangDocWillCrashAnd123 {};
+
+// CHECK-JSON: ThisStructHasANameThatResultsInAMangledNameThatIsExactly250CharactersLongThatIsSupposedToTestTheFilenameLengthLimitsWithinClangDocInOrdertoSeeifclangdocwillcrashornotdependingonthelengthofthestructIfTheLengthIsTooLongThenClangDocWillCrashAnd12.json
+// CHECK-JSON: {{[0-9A-F]*}}.json
+// CHECK-HTML: ThisStructHasANameThatResultsInAMangledNameThatIsExactly250CharactersLongThatIsSupposedToTestTheFilenameLengthLimitsWithinClangDocInOrdertoSeeifclangdocwillcrashornotdependingonthelengthofthestructIfTheLengthIsTooLongThenClangDocWillCrashAnd12.html
+// CHECK-HTML: {{[0-9A-F]*}}.html
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I thought we had a unit test for this? Am I misremembering?
Looking at the source: I am wrong and we never had one. |
The previous limit did not take into account filename extensions. Since the extensions we use (.yaml, .json, .html, .md) are at most 5 characters, we can lower the limit by 5. Also add some tests to make sure the rules are observed and don't explicitly crash clang-doc.
509159b
to
fc4ad9d
Compare
The previous limit did not take into account filename extensions. Since
the extensions we use (.yaml, .json, .html, .md) are at most 5
characters, we can lower the limit by 5.
Also add some tests to make sure the rules are observed and don't
explicitly crash clang-doc.