New option --copy COPYBOOK, to include a COPYBOOK #114

lefessan · 2023-10-11T13:48:28Z

This option provides the same behavior as --include FILE for gcc and clang, i.e. including a file (or a group of files) before parsing the source.
It can be used typically to include REPLACE statements.

codecov-commenter · 2023-10-11T13:54:30Z

Codecov Report

Merging #114 (993e3a4) into gcos4gnucobol-3.x (c0d64ad) will increase coverage by 0.02%.
The diff coverage is 86.53%.

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

@@                  Coverage Diff                  @@
##           gcos4gnucobol-3.x     #114      +/-   ##
=====================================================
+ Coverage              65.74%   65.76%   +0.02%     
=====================================================
  Files                     32       32              
  Lines                  59092    59133      +41     
  Branches               15575    15587      +12     
=====================================================
+ Hits                   38849    38890      +41     
+ Misses                 14262    14259       -3     
- Partials                5981     5984       +3

Files	Coverage Δ
cobc/flag.def	`100.00% <100.00%> (ø)`
cobc/help.c	`100.00% <100.00%> (ø)`
cobc/replace.c	`96.20% <ø> (+0.94%)`	⬆️
cobc/cobc.c	`72.51% <91.66%> (+0.12%)`	⬆️
cobc/codegen.c	`75.98% <60.00%> (-0.02%)`	⬇️
cobc/pplex.l	`72.67% <85.00%> (+0.05%)`	⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

GitMensch

I approve the idea and the general approach (that could be an issue if we don't discuss it up-front).

This needs some more checks and documentation, then would be fine.

Can you please also add --include to actually include the specified header in every generated .c file after the default includes?
This would be the "command line" part of https://sourceforge.net/p/gnucobol/feature-requests/176/ and would work identical with all C compilers as backends - and is just so related to this PR...

tests/testsuite.src/syn_copy.at

cobc/cobc.c

GitMensch

Only minor changes in the argument handling needed, otherwise good for SVN.

GitMensch · 2023-10-13T14:27:53Z

cobc/cobc.c

+	{
+		struct cb_text_list *l;
+		for (l = cb_copy_list; l; l=l->next){
+			char *name = cobc_strdup(l->text);


Do we need to duplicate the l->text? Why not using it directly?
If needed, then please use a local char name[COB_MINI_BUFF] here, this also removes the new failing under valgrind's memcheck when there is an error (because the err_exit leaves before we free the data).

The reason is that cb_copy_find_file() expects a non-const first argument (it modifies / into '\under Windows for example), whilecb_text_listcontains aconst char*` string.

I used the local array to remove the warning from valgrind.

Thank you for the explanation, adding a note that the buffer name point to is modified in its function doc is likely good (its enough to do before the commit to svn).

The change is good, was there also a reason (other than the late hour) to not use strcpy?

cobc/cobc.c

GitMensch · 2023-10-13T15:19:03Z

As I've just recognized that: this could also serve for some people to gather compile options "the other way" using a file with directives, which are then included by this option.

It would also provide a "similar" option to MicroFocus -C DIRECTIVES "FILE" (take the old file, copy as ".cpy" go in there and add $SET in front of each line, then compile in cobc with --copy your.dir.cpy.

GitMensch · 2023-10-13T15:20:33Z

Hm, and another one - mostly a question @lefessan: how does this apply to -fformat=auto, or better to say: shouldn't the auto-format be "reset" after each of the included --copy elements?

lefessan · 2023-10-13T16:20:19Z

It would also provide a "similar" option to MicroFocus -C DIRECTIVES "FILE" (take the old file, copy as ".cpy" go in there and add $SET in front of each line, then compile in cobc with --copy your.dir.cpy.

It should probably be part of a document of recipes to migrate from MicroFocus to GnuCOBOL. Unfortunately, the localisation of errors would make it difficult to debug in case of problem, it would be better to call a directive entry point in the parser for each directives in a list, juste before including the copies.

GitMensch · 2023-10-13T16:31:44Z

it would be better to call a directive entry point in the parser for each directives in a list, just before including the copies

That would be interesting, especially if it handles the different directive variants. Something to keep in mind for later.

lefessan · 2023-10-13T16:38:11Z

Hm, and another one - mostly a question @lefessan: how does this apply to -fformat=auto, or better to say: shouldn't the auto-format be "reset" after each of the included --copy elements?

Good point. I just checked: if the format is auto, the detection is performed on the main target, the format is then set, and all copybooks are expected to be of the same format. I think it's a reasonable behavior.

GitMensch · 2023-10-13T16:46:33Z

I agree, that's a reasonable approach, possibly also to suggest users to use format-independent COBOL in those --copy included copybooks ;-)

GitMensch · 2023-10-20T05:44:48Z

As these are new options: please add them to the NEWS. I suggest to do that here already to not forget it on the later commit.

lefessan · 2023-10-28T08:51:14Z

I just added the NEWS section.

I am still not comfortable with using absolute paths with --include. Recently, we discussed about using Docker to provide simpler multi-platform installations for GnuCOBOL: in my experience, it works pretty well, but only if all your paths are relative (the idea is to mount the local directory somewhere and run the program inside the container in that directory, so that it can access all local files). So, the --include "$PWD/FILE.h" will fail in such a context, since the absolute path is not the one in the Docker image.

If we cannot make a relative path the default, I added a commit to add a COBC_BUILD_INPLACE env variable that makes the C file to be generated and build in the local directory. It's not my preferred solution, but it would allow the Docker version to work correctly by setting it inside and outside the container.

GitMensch · 2023-10-28T09:02:07Z

Can you please explain where you see the problem with something like --include "FILE.h" -I $PWD or --include "FILE.h" -g or --include "FILE.h" --save-temps?

lefessan · 2023-10-28T09:15:35Z

I didn't notice that -g could be used for that.

For me, the behavior of --include FILE.h should not depend on whether it is used with or without another unrelated option. Indeed, --include FILE.h -g will probably be the set of options used by most users, so it will work without $PWD, but then, they will not understand why removing -g suddenly make their build fail.

For now, I only see scenarios where the option would only be used in development mode (so, with -g), but my experience is that one day or another, somebody will find a way to try to use this option in an unexpected way in release mode too (to replace some calls by macros ?), so we should let the door opened...

GitMensch · 2023-10-28T09:28:28Z

so we should let the door opened...

We do with --save-temps or -I $TMPDIR. You may consider checking gnucobol.tex if those three possibilities are mentioned there or should be mentioned.
I'm not sure we need that at all, because, as I've said, people most commonly use that option with including a file from the system include directories; for me a single half sentence about this "note that to find the file you may need to pass an appropriate -I" or alike would be enough.

GitMensch · 2023-10-28T16:53:01Z

What's the benefit of COBC_BUILD_INPLACE over --save-temps?
I guess the difference is it would place the intermediate files locally, but then deletes them afterwards, right?

lefessan · 2023-10-30T10:38:13Z

Yes, it does not leave intermediate files in the local dir. (FWIW, my latency is high since last week as I am in a two-week vacation with my kids... and I didn't have as much time as I expected to hack on my spare time :-) )

GitMensch

Please adjust the Changelog to reference FR #176 (and close that as soon as the change is committed to svn).
Please drop COBC_BUILD_INPLACE and replace as noted in the previous posts.

Afterwards this can go into svn (ideally after @ddeclerck's changes).

lefessan · 2024-01-22T13:41:47Z

Please drop COBC_BUILD_INPLACE and replace as noted in the previous posts.

I am still not fond of using --save-temps to get this behavior, as it will pollute the local directory with unwanted intermediate files. Is it possible to keep it ?

GitMensch · 2024-01-22T14:10:51Z

Isn't it enough to have -I . if you want the local path?

In any case: #112 can be integrated upstream before.

NEWS

cobc/cobc.c

GitMensch

Looks ready to me. One more solved :-)

tests/testsuite.src/used_binaries.at

cobc/ChangeLog

…source file

This option has two side-effect: * the C file is compiled in the project directory (so that the included file can be found locally) * no prototype is generated, so the new header file must contain a prototype for all statically called functions

lefessan · 2024-01-31T18:07:54Z

Merged.

GitMensch · 2024-01-31T20:54:48Z

Thank you. I've just recognized that there's one missing part from FR # 176: Back in the discussions then the point was mot sources that call into C always need the same C header, and therefore a new directive was the goal.

Can you or @engboris have a look at adding the >>IMP INCLUDE directive and close the related FR by that?

GitMensch · 2024-03-12T11:44:05Z

reopen for the related missing part of the FR

engboris · 2024-04-25T16:01:00Z

Thank you. I've just recognized that there's one missing part from FR # 176: Back in the discussions then the point was mot sources that call into C always need the same C header, and therefore a new directive was the goal.

Can you or @engboris have a look at adding the >>IMP INCLUDE directive and close the related FR by that?

I'm currently working on it. What should happen if both the compiler option and the directive are defined? Should one be ignored? Both be considered?

I also plan to allow appending on text_list (probably restricted to cb_include_file_list) in cobc/cobc.h because it doesn't look like I'm able to do it from ppparse.y where the directive is parsed and defined.

EDIT : just noticed there are functions for that for the second paragraph of my answer

GitMensch · 2024-04-25T16:35:42Z

What should happen if both the compiler option and the directive are defined?

You may also have multiple >> IMP INCLUDE - each one (as well as the command line option) add to the list, the list is restored before the next input source file is handled (similar to source format).

GitMensch · 2024-09-17T16:38:17Z

ping @engboris

engboris · 2024-09-17T17:21:33Z

Hello @GitMensch
Thank you for your reminder.

Maybe there is a misunderstanding? I have a PR here #143 (which I believe is almost completed) and I was waiting for your feedback.

Is there something you expected me to do that I didn't notice?

ddeclerck · 2024-10-02T15:00:25Z

Shall we close this one again, since it is now superseded by #143 ?

GitMensch · 2024-10-02T18:18:43Z

Yes, it is fine to close that as only the directive is missing and this is what the related PR is about (not sure when I find the time to inspect the other current PRs though...).

GitMensch requested changes Oct 11, 2023

View reviewed changes

tests/testsuite.src/syn_copy.at Outdated Show resolved Hide resolved

tests/testsuite.src/syn_copy.at Show resolved Hide resolved

cobc/cobc.c Outdated Show resolved Hide resolved

cobc/cobc.c Outdated Show resolved Hide resolved

lefessan force-pushed the z-2023-10-11-copy-arg branch 5 times, most recently from e5f0129 to 49ec0d8 Compare October 13, 2023 12:11

GitMensch approved these changes Oct 13, 2023

View reviewed changes

lefessan force-pushed the z-2023-10-11-copy-arg branch 2 times, most recently from 9d017b1 to 52c386a Compare October 13, 2023 21:46

GitMensch added the Ready for SVN label Oct 19, 2023

lefessan force-pushed the z-2023-10-11-copy-arg branch from 14f72d6 to 993e3a4 Compare October 28, 2023 08:52

GitMensch requested changes Jan 20, 2024

View reviewed changes

lefessan force-pushed the z-2023-10-11-copy-arg branch from 993e3a4 to 2f012f0 Compare January 22, 2024 15:53

GitMensch reviewed Jan 22, 2024

View reviewed changes

NEWS Outdated Show resolved Hide resolved

GitMensch reviewed Jan 22, 2024

View reviewed changes

cobc/cobc.c Outdated Show resolved Hide resolved

lefessan force-pushed the z-2023-10-11-copy-arg branch from 2f012f0 to 9987137 Compare January 25, 2024 22:17

GitMensch approved these changes Jan 25, 2024

View reviewed changes

tests/testsuite.src/used_binaries.at Outdated Show resolved Hide resolved

cobc/ChangeLog Show resolved Hide resolved

lefessan added 2 commits January 31, 2024 18:54

New option --copy COPYBOOK, to include a COPYBOOK before reading the …

c4425a2

…source file

lefessan force-pushed the z-2023-10-11-copy-arg branch from 1083820 to 82b9448 Compare January 31, 2024 17:55

lefessan added the Merged in SVN label Jan 31, 2024

lefessan closed this Jan 31, 2024

GitMensch reopened this Mar 12, 2024

GitMensch added reopened (merged) already merged, but reopened and removed Merged in SVN labels Mar 12, 2024

engboris mentioned this pull request Apr 25, 2024

Add >>IMP INCLUDE directive #143

Open

GitMensch added Merged in SVN and removed reopened (merged) already merged, but reopened Ready for SVN labels Oct 2, 2024

GitMensch closed this Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New option --copy COPYBOOK, to include a COPYBOOK #114

New option --copy COPYBOOK, to include a COPYBOOK #114

lefessan commented Oct 11, 2023

codecov-commenter commented Oct 11, 2023 •

edited

Loading

GitMensch left a comment

GitMensch left a comment

GitMensch Oct 13, 2023

lefessan Oct 13, 2023

GitMensch Oct 14, 2023

GitMensch commented Oct 13, 2023

GitMensch commented Oct 13, 2023

lefessan commented Oct 13, 2023

GitMensch commented Oct 13, 2023

lefessan commented Oct 13, 2023

GitMensch commented Oct 13, 2023

GitMensch commented Oct 20, 2023

lefessan commented Oct 28, 2023

GitMensch commented Oct 28, 2023 •

edited

Loading

lefessan commented Oct 28, 2023

GitMensch commented Oct 28, 2023

GitMensch commented Oct 28, 2023

lefessan commented Oct 30, 2023

GitMensch left a comment

lefessan commented Jan 22, 2024

GitMensch commented Jan 22, 2024

GitMensch left a comment

lefessan commented Jan 31, 2024

GitMensch commented Jan 31, 2024

GitMensch commented Mar 12, 2024

engboris commented Apr 25, 2024 •

edited

Loading

GitMensch commented Apr 25, 2024

GitMensch commented Sep 17, 2024

engboris commented Sep 17, 2024

ddeclerck commented Oct 2, 2024

GitMensch commented Oct 2, 2024

New option --copy COPYBOOK, to include a COPYBOOK #114

New option --copy COPYBOOK, to include a COPYBOOK #114

Conversation

lefessan commented Oct 11, 2023

codecov-commenter commented Oct 11, 2023 • edited Loading

Codecov Report

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch left a comment

Choose a reason for hiding this comment

GitMensch Oct 13, 2023

Choose a reason for hiding this comment

lefessan Oct 13, 2023

Choose a reason for hiding this comment

GitMensch Oct 14, 2023

Choose a reason for hiding this comment

GitMensch commented Oct 13, 2023

GitMensch commented Oct 13, 2023

lefessan commented Oct 13, 2023

GitMensch commented Oct 13, 2023

lefessan commented Oct 13, 2023

GitMensch commented Oct 13, 2023

GitMensch commented Oct 20, 2023

lefessan commented Oct 28, 2023

GitMensch commented Oct 28, 2023 • edited Loading

lefessan commented Oct 28, 2023

GitMensch commented Oct 28, 2023

GitMensch commented Oct 28, 2023

lefessan commented Oct 30, 2023

GitMensch left a comment

Choose a reason for hiding this comment

lefessan commented Jan 22, 2024

GitMensch commented Jan 22, 2024

GitMensch left a comment

Choose a reason for hiding this comment

lefessan commented Jan 31, 2024

GitMensch commented Jan 31, 2024

GitMensch commented Mar 12, 2024

engboris commented Apr 25, 2024 • edited Loading

GitMensch commented Apr 25, 2024

GitMensch commented Sep 17, 2024

engboris commented Sep 17, 2024

ddeclerck commented Oct 2, 2024

GitMensch commented Oct 2, 2024

codecov-commenter commented Oct 11, 2023 •

edited

Loading

GitMensch commented Oct 28, 2023 •

edited

Loading

engboris commented Apr 25, 2024 •

edited

Loading