-
Notifications
You must be signed in to change notification settings - Fork 640
fix(cxx): handle complex template specializations in scope stack #4348
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
base: master
Are you sure you want to change the base?
fix(cxx): handle complex template specializations in scope stack #4348
Conversation
Fix assertion failure in cxxScopePushTop when parsing member function pointer template specializations. The debug assertion requiring tokens to be simple identifiers was too strict for complex template constructs like "TestUtil<MEMTYPE CLASS::*>::isNull". Replace the strict assertion with adaptive logic that converts non-identifier tokens to identifier type when they have valid text content, following the established pattern used elsewhere in the parser for similar cases. Fixes universal-ctags#4344
The CI was failing due to missing trailing newline in expected output. Add proper line ending to match ctags output format.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4348 +/- ##
=======================================
Coverage 85.87% 85.87%
=======================================
Files 252 252
Lines 62597 62598 +1
=======================================
+ Hits 53755 53756 +1
Misses 8842 8842 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| isNull input.cpp /^ static bool isNull(const T&) { return false; }$/;" f struct:TestUtil typeref:typename:bool file: | ||
| TestUtil input.cpp /^struct TestUtil<T*> {$/;" s file: | ||
| isNull input.cpp /^ static bool isNull(T* f) { return 0 == f; }$/;" f struct:TestUtil typeref:typename:bool file: | ||
| isNull input.cpp /^TestUtil<MEMTYPE CLASS::*>::isNull(MEMTYPE CLASS::* f)$/;" f class:TestUtil::* typeref:typename:bool |
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.
class:TestUtil::*
Do you think we need the last *?
I also tried to fix the original issue.
diff --git a/parsers/cxx/cxx_parser_function.c b/parsers/cxx/cxx_parser_function.c
index d8126019f..5e15e51e1 100644
--- a/parsers/cxx/cxx_parser_function.c
+++ b/parsers/cxx/cxx_parser_function.c
@@ -1591,10 +1591,21 @@ int cxxParserEmitFunctionTags(
{
CXXToken * pScopeId = pInfo->pScopeStart;
- pInfo->pScopeStart = cxxTokenChainNextTokenOfType(
+ while (1)
+ {
+ pInfo->pScopeStart = cxxTokenChainNextTokenOfType(
pInfo->pScopeStart,
- CXXTokenTypeMultipleColons
- );
+ CXXTokenTypeMultipleColons|CXXTokenTypeSmallerThanSign);
+ /* ??? */
+ CXX_DEBUG_ASSERT(pInfo->pScopeStart,"We should have found a next token here");
+ if (cxxTokenTypeIs(pInfo->pScopeStart, CXXTokenTypeMultipleColons))
+ break;
+ if (cxxTokenTypeIs(pInfo->pScopeStart, CXXTokenTypeSmallerThanSign)) {
+ pInfo->pScopeStart = cxxTokenChainSkipToEndOfTemplateAngleBracket(pInfo->pScopeStart);
+ /* if (pInfo->pScopeStart == NULL) what I should do? */
+ CXX_DEBUG_ASSERT(pInfo->pScopeStart,"We should have found a next token here <>");
+ }
+ }
CXX_DEBUG_ASSERT(pInfo->pScopeStart,"We should have found a next token here");
With this change, the parser can skip <...> in the scope.
My version passed most of your new test cases. The exception is:
[yamato@dev64]~/var/ctags-github% misc/review
1) <a>ccept
2) <s>kip (or <n>)
3) <S>hell
4) <d>iff
5) show args.<c>tags
6) show <i>nput
7) <e>xpected tags
8) <q>uit
[1/1 [ Units/parser-cxx.r/template-member-function-pointer-scope.d ]]? =
--- ./Units/parser-cxx.r/template-member-function-pointer-scope.d/expected.tags 2025-11-14 03:32:44.593286373 +0900
+++ /home/yamato/var/ctags-github/Units/parser-cxx.r/template-member-function-pointer-scope.d/FILTERED.tmp 2025-11-14 03:33:58.558081369 +0900
@@ -5 +5 @@
-isNull input.cpp /^TestUtil<MEMTYPE CLASS::*>::isNull(MEMTYPE CLASS::* f)$/;" f class:TestUtil::* typeref:typename:bool
+isNull input.cpp /^TestUtil<MEMTYPE CLASS::*>::isNull(MEMTYPE CLASS::* f)$/;" f class:TestUtil typeref:typename:bool
---
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.
Do you think we need the last
*?
I think there's value on having it, but alas I'm +0.1 on it, so don't feel strongly about it.
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.
Do you think class:TestUtil<MEMTYPE CLASS::*>:: is better than class:TestUtil::*?
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.
I'd say yes.
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.
I'd say yes.
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.
I have looked into other test cases.
In expected.tags files in the test cases, Tokens in <...> are just dropped.
Till someone having a strong opinion appears, I will choose class:TestUtil.
I will make a pull request based on your work.
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.
You can always update this PR rather than create a brand new one 🤔
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.
You can always update this PR rather than create a brand new one 🤔
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.
You can always update this PR rather than create a brand new one 🤔
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.
Fix assertion failure in cxxScopePushTop when parsing member function pointer template specializations. The debug assertion requiring tokens to
be simple identifiers was too strict for complex template constructs like
"TestUtil<MEMTYPE CLASS::*>::isNull".Replace the strict assertion with adaptive logic that converts non-identifier tokens to identifier type when they have valid text content, following the
established pattern used elsewhere in the parser for similar cases.
Fixes #4344