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

fix SEGFAULT in checking prototype arguments #133

Conversation

lefessan
Copy link
Member

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.85%. Comparing base (a2a51fd) to head (9f7df04).
Report is 99 commits behind head on gcos4gnucobol-3.x.

Files with missing lines Patch % Lines
cobc/parser.y 80.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #133      +/-   ##
=====================================================
- Coverage              65.86%   65.85%   -0.01%     
=====================================================
  Files                     32       32              
  Lines                  59481    59483       +2     
  Branches               15708    15709       +1     
=====================================================
- Hits                   39177    39175       -2     
- Misses                 14283    14290       +7     
+ Partials                6021     6018       -3     

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

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

thanks for the test and the fix, only a minor change to the testsuite, an added ChangeLog entry and that's good to go

@lefessan lefessan force-pushed the z-2024-01-26-fix-prototype-check branch 2 times, most recently from 743bbd3 to 37bc01f Compare January 28, 2024 17:43
@GitMensch
Copy link
Collaborator

GitMensch commented Jan 28, 2024

I see, we expect WORD and have a LITERAL.

For level 78 (likely also for level 01 constants) we have several places that result in a strange error message (and like in that place a bit misleading position of the error message). A way to solve that would be passing an extra token type, but then we'd have to add something like the following - and use it in nearly all places where we have LITERAL and some, like that, where we currently use WORD.

literal: LITERAL { $$ = $1; } | CONSTANT { $$ = CB_TREE (CB_CONST_USER ($1)->literal;) }

That's definitely too much for this PR.
As the parser error leads to a "totally different" code path: please leave lvl78 in the test, but commented out with a note.

@lefessan
Copy link
Member Author

@engboris Maybe that could be a first job for you ? If I understand well, the idea is to remove 78-level constants from the lexer, and replace all the places where they can replace a LITERAL by a rule that also accepts a WORD and checks if it is a 78-level ?

@GitMensch
Copy link
Collaborator

@lefessan that would be quite a big change and @engboris already took one of the other issues (towards stateless runtime), which is good to be finally handled.

@lefessan
Copy link
Member Author

I commented out the level 78 from the prototype call.

@GitMensch
Copy link
Collaborator

Good as is now. For the additional part of the 78 handling - please create an issue in the wish-list tracker with as much technical information as reasonable for someon to work on this later.

@GitMensch
Copy link
Collaborator

@lefessan @ddeclerck Who does the commit to SVN now?

@ddeclerck
Copy link
Contributor

Fabrice might be a bit busy at the moment, I'll take care of it (tomorrow I guess).

@ddeclerck ddeclerck force-pushed the z-2024-01-26-fix-prototype-check branch from 43afbdc to 9f7df04 Compare March 13, 2024 14:06
@ddeclerck
Copy link
Contributor

Merged in SVN at revision 5227.
(@GitMensch Can you reattribute it to Fabrice ?)

@GitMensch
Copy link
Collaborator

metadata updated, no bug issue to be closed (nut that's quite an exotic point to reach, and described enough in the ChangeLog - not necessary to create a bug report)

On to the next PRs :-)

@GitMensch GitMensch closed this Mar 13, 2024
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.

4 participants