OCamlPro / gnucobol

A clone of the sourceforge GnuCOBOL compiler from COBOL to C.
https://get-superbol.com
GNU Lesser General Public License v3.0
16 stars 20 forks source link

fix SEGFAULT in checking prototype arguments #133

Closed lefessan closed 4 months ago

codecov-commenter commented 6 months ago

Codecov Report

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

Project coverage is 65.85%. Comparing base (a2a51fd) to head (9f7df04).

Files Patch % Lines
cobc/parser.y 80.00% 0 Missing and 1 partial :warning:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GitMensch commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

I commented out the level 78 from the prototype call.

GitMensch commented 5 months ago

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 commented 4 months ago

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

ddeclerck commented 4 months ago

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

ddeclerck commented 4 months ago

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

GitMensch commented 4 months ago

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 :-)