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

Add >>IMP INCLUDE directive #143

Open
wants to merge 11 commits into
base: gcos4gnucobol-3.x
Choose a base branch
from

Conversation

engboris
Copy link
Contributor

@engboris engboris commented Apr 25, 2024

Closes #114 and completes FR176 on SourceForge.

A new directive >> IMP INCLUDE FILE_1 ... FILE_n is introduced, allowing to include multiple C headers. Only files with name finishing by .h are allowed.

I'm not sure about that but I marked the feature with /* GnuCOBOL 3.3 extension*/.

@engboris engboris added the enhancement New feature or request label Apr 25, 2024
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 this first take - please copy the tests for --include and adjust those for the directive. In gnbucobol.texi and NEWS add a remark in the --include description, that the directive can be used as an alternative.

cobc/pplex.l Outdated Show resolved Hide resolved
cobc/ppparse.y Outdated Show resolved Hide resolved
cobc/ppparse.y Outdated Show resolved Hide resolved
cobc/pplex.l Outdated Show resolved Hide resolved
@GitMensch
Copy link
Collaborator

GitMensch commented Apr 29, 2024 via email

@engboris engboris force-pushed the engboris-imp-directive branch 4 times, most recently from c035033 to 7bb6097 Compare April 30, 2024 16:07
cobc/pplex.l Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
tests/testsuite.src/syn_misc.at Outdated Show resolved Hide resolved
@engboris engboris force-pushed the engboris-imp-directive branch 3 times, most recently from 2ad6625 to 7b94002 Compare May 3, 2024 15:13
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.

still some requested changes - but we're on the finish line with this PR

NEWS Outdated Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
cobc/cobc.c Outdated Show resolved Hide resolved
cobc/ChangeLog Outdated Show resolved Hide resolved
tests/testsuite.src/syn_misc.at Show resolved Hide resolved
tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
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.

"approved with minor changes"

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.

changes to the testcases needed, overal fine

tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved
tests/testsuite.src/syn_misc.at Show resolved Hide resolved
cobc/codegen.c Outdated Show resolved Hide resolved
@engboris engboris force-pushed the engboris-imp-directive branch 5 times, most recently from 331e050 to 0df9e5b Compare June 13, 2024 12:37
@engboris engboris closed this Jun 13, 2024
@ddeclerck ddeclerck reopened this Jun 13, 2024
@engboris
Copy link
Contributor Author

@GitMensch Could you check this? It was almost finished and it was a rather minor change but I wasn't sure about the tests. Thank you!

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.

code changes requested for @engboris - I'll adjust the used_binaries.at, so please don't force-push

cobc/ChangeLog Outdated Show resolved Hide resolved
cobc/cobc.h Outdated Show resolved Hide resolved
cobc/pplex.l Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
cobc/codegen.c Outdated
Comment on lines 1844 to 1848
if (last != NULL) {
cobc_free (last->text);
cobc_free (last);
}
last = l;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently the memory was allocated with cobc_malloc and would need to be always freed, and that must also be done when there is no codegen (preprocess/syntaxy-check only) so must be done outside of codegen.

I think the parse_ memory functions would be more reasonable and drop the need to execute the free, also removing the need to keep the "last" element here; the only thing missing then is to initialize cb_include_file_list_directive at the right place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, I should free those variables outside/independently of codegen but still after the running of codegen? Maybe after ylex_clear_all () in cobc.c and add a parse_clear_all () function or something like that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The thought was to drop all the explicit cobc_free calls and move this to "parser memory" using cobc_parse_malloc instead for the allocation; then, potentially at ylex_clear_all or clear_local_codegen_vars unset the var...

Can you please check if this works?

Note: you currently allocate cb_include_file_list_directive with cobc_parse_strdup - so it is parser allocated already and will automatically be freed - you may only need to drop setting the "last" attribute (or create a separate structure) and the free calls here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you may only need to drop setting the "last" attribute (or create a separate structure) and the free calls here.

Looks like it works with that with no leak from valgrind. I didn't unset any variable (tell me if it is necessary).

tests/testsuite.src/syn_misc.at Outdated Show resolved Hide resolved
cobc/ppparse.y Show resolved Hide resolved
@GitMensch

This comment was marked as resolved.

cobc/ppparse.y Outdated Show resolved Hide resolved
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.

overall a good state, we should be able to add the missing pieces and be able to upstream that, soon

cobc/cobc.h Outdated Show resolved Hide resolved
cobc/codegen.c Outdated
Comment on lines 1844 to 1848
if (last != NULL) {
cobc_free (last->text);
cobc_free (last);
}
last = l;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The thought was to drop all the explicit cobc_free calls and move this to "parser memory" using cobc_parse_malloc instead for the allocation; then, potentially at ylex_clear_all or clear_local_codegen_vars unset the var...

Can you please check if this works?

Note: you currently allocate cb_include_file_list_directive with cobc_parse_strdup - so it is parser allocated already and will automatically be freed - you may only need to drop setting the "last" attribute (or create a separate structure) and the free calls here.

cobc/ppparse.y Outdated Show resolved Hide resolved
cobc/scanner.l Outdated Show resolved Hide resolved
tests/testsuite.src/syn_misc.at Show resolved Hide resolved
@codecov-commenter
Copy link

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

Codecov Report

Attention: Patch coverage is 73.07692% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.80%. Comparing base (2c000f5) to head (1336ded).
Report is 15 commits behind head on gcos4gnucobol-3.x.

Files with missing lines Patch % Lines
cobc/scanner.l 74.19% 3 Missing and 5 partials ⚠️
cobc/codegen.c 50.00% 1 Missing and 2 partials ⚠️
cobc/pplex.l 57.14% 0 Missing and 3 partials ⚠️

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

Additional details and impacted files
@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #143      +/-   ##
=====================================================
- Coverage              77.56%   67.80%   -9.76%     
=====================================================
  Files                     33       33              
  Lines                  60230    60413     +183     
  Branches               15834    15888      +54     
=====================================================
- Hits                   46717    40966    -5751     
- Misses                 13513    13538      +25     
- Partials                   0     5909    +5909     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants