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

c-based: fix to handle edge case #3796

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

jafl
Copy link
Contributor

@jafl jafl commented Aug 16, 2023

where "character" returned is a symbol (> 0xff)

@jafl jafl changed the title c-based: fix to handle edge case where "character" returned is a symb… c-based: fix to handle edge case Aug 16, 2023
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Patch coverage: 90.32% and project coverage change: -0.01% ⚠️

Comparison is base (cea67b9) 85.07% compared to head (f3a2e44) 85.06%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3796      +/-   ##
==========================================
- Coverage   85.07%   85.06%   -0.01%     
==========================================
  Files         226      226              
  Lines       53851    53857       +6     
==========================================
+ Hits        45814    45816       +2     
- Misses       8037     8041       +4     
Files Changed Coverage Δ
parsers/c-based.c 81.88% <83.33%> (-0.35%) ⬇️
parsers/cpreprocessor.c 95.64% <90.90%> (-0.10%) ⬇️
parsers/cxx/cxx_parser_tokenizer.c 93.02% <100.00%> (-0.10%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jafl jafl requested review from masatake and techee August 16, 2023 03:17
@masatake
Copy link
Member

Thank you.
Related to #3771. I'm working on this topic very slowly.

@jafl
Copy link
Contributor Author

jafl commented Aug 17, 2023

@masatake I can tackle this issue and try to finish it. Clearly, anywhere that calls cppGetc() needs to be audited to handle c > 0xff. What other pieces are there that need to be fixed?

@jafl jafl force-pushed the c-based-fix-for-string-symbol branch from fce288e to 884ef4e Compare August 17, 2023 04:55
@jafl
Copy link
Contributor Author

jafl commented Aug 17, 2023

I simplified my original changes, because I realized that cppUngetc is just fine with c > 0xff

@jafl
Copy link
Contributor Author

jafl commented Aug 17, 2023

Failed checks are system issues, not code.

@leleliu008
Copy link
Member

Github Actions may fail for network issues, If you encounter this situation next time, just rerun it. It usually would become green after rerunning.

@jafl jafl force-pushed the c-based-fix-for-string-symbol branch 2 times, most recently from 64d6b67 to 7227383 Compare August 18, 2023 03:56
@jafl
Copy link
Contributor Author

jafl commented Aug 18, 2023

@masatake Here is my proposal for fixing the issue. I created cStringPut as a smart wrapper around vStringPut and made cppUngetc smarter. If you are ok with this, I will convert all vStringPut to cStringPut in ./cxx/cxx_parser_tokenizer.c, cpreprocessor.c, vera.c, and c-based.c. (This is the list of files from #3771)

I only changed the minimal number of calls so far, to minimize the size of the commit and make it easier to review.

@masatake
Copy link
Member

Give me some time.

@jafl jafl force-pushed the c-based-fix-for-string-symbol branch from 7227383 to 5e9053d Compare August 19, 2023 05:34
@masatake
Copy link
Member

@jafl Looks fine.
Before merging this, I would like to merge #3804.
Could you consider updating cStringPut in your pull request after I merge #3804?

@jafl
Copy link
Contributor Author

jafl commented Aug 19, 2023

@masatake Sure. Let me know when you merge #3804, and I'll rebase.

@jafl
Copy link
Contributor Author

jafl commented Aug 19, 2023

@masatake Is it ok if I convert all vStringPut to cStringPut in all the files listed in #3771 ?

@masatake
Copy link
Member

I merge #3804. Could you rebase this?
You must update the definition of cStringPut.

@masatake Is it ok if I convert all vStringPut to cStringPut in all the files listed in #3771 ?

At this time, could you update only vStringPut in c-based.c?
I don't have enough time to review the changes for other files at once.
I deleted c.c. Even you updated c-based.c, users get the benefit of the change.

For fixing #3771, I have been thinking about the way of handling EOF.
When I read this pull request, I recognized that thinking about EOF is insufficient. Thank you for the pull request.

I have been finding my mistakes in cpreprocess.c because of my limited understanding of the file.

@jafl jafl force-pushed the c-based-fix-for-string-symbol branch from 5e9053d to f3a2e44 Compare August 20, 2023 16:07
@jafl jafl requested a review from masatake August 20, 2023 16:08
@jafl jafl merged commit d104ee5 into universal-ctags:master Aug 20, 2023
38 of 39 checks passed
@jafl jafl deleted the c-based-fix-for-string-symbol branch August 20, 2023 21:49
@masatake
Copy link
Member

Thank you.

masatake added a commit to masatake/ctags that referenced this pull request Dec 18, 2023
Close universal-ctags#3771.
Did the same as universal-ctags#3796.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
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.

3 participants