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

#3388 Bugfix for cpp template function return types #3416

Conversation

delsner
Copy link
Contributor

@delsner delsner commented Jun 20, 2022

Fixes issue #3388, where the C++ parser does not correctly parse template function return types and therefore misses functions.

This PR removes C++ template parsing code that was originally written to support C++03-specific template syntax. However, as described in #3388 this syntax is not actually valid C++03. Yet, this code prevents correctly parsing non-primitive template function return types.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #3416 (7dd0cdf) into master (7c12820) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3416      +/-   ##
==========================================
- Coverage   83.22%   83.21%   -0.02%     
==========================================
  Files         218      218              
  Lines       52449    52439      -10     
==========================================
- Hits        43650    43635      -15     
- Misses       8799     8804       +5     
Impacted Files Coverage Δ
parsers/cxx/cxx_parser_template.c 74.81% <ø> (-2.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c12820...7dd0cdf. Read the comment docs.

@masatake masatake requested a review from pragmaware June 21, 2022 09:39
@masatake masatake assigned masatake and pragmaware and unassigned masatake Jun 21, 2022
@masatake
Copy link
Member

Could you add a test case?
http://docs.ctags.io/en/latest/testing-parser.html#units-test-facility explains how to write a test case for "Units".

@delsner
Copy link
Contributor Author

delsner commented Jun 23, 2022

sure, I'll add a unit test when I get to it!

@delsner
Copy link
Contributor Author

delsner commented Jun 24, 2022

The codecov coverage threshold check is failing and there also seems to be a flaky test here: https://github.com/universal-ctags/ctags/runs/7044140203; I also added a small fix for a comment that was introduced in #3398 (see aaf6ca2)

@delsner
Copy link
Contributor Author

delsner commented Jun 24, 2022

@masatake any more comments on this PR?

@masatake
Copy link
Member

7560461
7560461
96f45ea

These can be squashed into one commit. We should not record your try-and-error commits to the ctags history if possible.

About e7fbffe, I would like you to start the header of the commit log with "C++: ...".

Instead of putting the string #3388 to the header, could you put Close #3388. to the body of the commit log?

You explained the change in the first comment of this pull request.
This is good. However, the commit log is too short. Could you write the commit log as much like the initial comment like 55ba726.

@masatake
Copy link
Member

I forgot to write the most important message; thank you.

I'm not sure whether I can ask this or not but...
You deleted code from cxxParserParseTemplateAngleBracketsInternal().
You will see many comments in cxxParserParseTemplateAngleBracketsInternal().
Could you try making the comments consistent with the deletion?

@delsner
Copy link
Contributor Author

delsner commented Jun 24, 2022

thanks for the feedback, I'll incorporate it tomorrow. I expected to squash the entire PR into a single commit after all, but I'll rewrite the history as requested!

@delsner delsner force-pushed the bugfix/#3388-fix-template-function-return-type branch from aaf6ca2 to 586eaee Compare June 25, 2022 09:58
@delsner
Copy link
Contributor Author

delsner commented Jun 25, 2022

@masatake feel free to check again. Regarding making the comments more consistent: I added an explanation on why parsing non-nested greater than signs is not necessary (syntax error) and left a note pointing to the issue where the deleted code used to be.

@masatake
Copy link
Member

(The failure on Cygwin is nothing to do with this pull request.)

@masatake
Copy link
Member

I'm trying to fix the failures on Cygwin.

@masatake
Copy link
Member

Could you rebase your change on the LATEST main branch?

@delsner delsner force-pushed the bugfix/#3388-fix-template-function-return-type branch from 586eaee to c8de22f Compare June 26, 2022 11:38
@delsner
Copy link
Contributor Author

delsner commented Jun 26, 2022

Could you rebase your change on the LATEST main branch?

done, let's see!

@masatake
Copy link
Member

I read the changes. Thank you. It looks good to me. I will merge this pull request.
Before merging, could you see the minor requests?

delsner and others added 3 commits June 26, 2022 23:06
…pp compilers

Close universal-ctags#3388.

Removes C++ template parsing code that was originally written to support C++03-specific template syntax.
However, as described in universal-ctags#3388, this syntax is not actually valid C++03 syntax.
Yet, the parsing code removed in this commit prevents correctly parsing non-primitive template function return types.
@delsner delsner force-pushed the bugfix/#3388-fix-template-function-return-type branch from c8de22f to 7dd0cdf Compare June 26, 2022 21:08
@masatake masatake merged commit af8c172 into universal-ctags:master Jun 26, 2022
@masatake
Copy link
Member

Thank you very much.

@delsner delsner deleted the bugfix/#3388-fix-template-function-return-type branch June 27, 2022 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants