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

Support more compiler directives #257

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

nberth
Copy link
Collaborator

@nberth nberth commented Apr 2, 2024

Adds support for >>DEFINE and simple (cascading) conditional compilation directives. >>SET directives are parsed but not interpreted yet. >>EVALUATE/>>WHEN shall be added in a subsequent PR.

@emilienlemaire
Copy link
Contributor

Would this handle #153 ?

@nberth
Copy link
Collaborator Author

nberth commented Apr 4, 2024

Would this handle #153 ?

No, the >>D is a floating indicator, not a compiler directive per se. Introducing that one may need quite some care (and we don't have that many examples of its usage I think), so it's left for later.

@emilienlemaire
Copy link
Contributor

We would like to be able to parse the whole https://github.com/OCamlPro/gnucobol-contrib/tree/1618cb70d3855fd56a17bb2522c421e38993fc6d/samples/worldcities folder, (with the fixes from OCamlPro/gnucobol-contrib#1), and this is used in some files

@nberth
Copy link
Collaborator Author

nberth commented Apr 4, 2024

We would like to be able to parse the whole https://github.com/OCamlPro/gnucobol-contrib/tree/1618cb70d3855fd56a17bb2522c421e38993fc6d/samples/worldcities folder, (with the fixes from OCamlPro/gnucobol-contrib#1), and this is used in some files

I see in there it's not used in floating position… so maybe Boris's suggestion could work (as a temporary measure).

@GitMensch
Copy link
Contributor

No, the >>D is a floating indicator, not a compiler directive per se. Introducing that one may need quite some care (and we don't have that many examples of its usage I think), so it's left for later.

I think only GnuCOBOL supports that and the features is removed in COBOL 2014, so handling this specific one is likely not important.

@nberth nberth force-pushed the more-preproc-directives branch 4 times, most recently from 0a6c7c9 to 460b4e3 Compare April 5, 2024 13:51
@nberth nberth marked this pull request as ready for review April 5, 2024 13:56
Copy link
Contributor

@emilienlemaire emilienlemaire left a comment

Choose a reason for hiding this comment

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

Seems all good to me, but one question concerning a test failure

test/output-tests/run_misc.expected Show resolved Hide resolved
Copy link
Contributor

@emilienlemaire emilienlemaire left a comment

Choose a reason for hiding this comment

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

All good for me 👌

@GitMensch
Copy link
Contributor

>>EVALUATE/>>WHEN shall be added in a subsequent PR.

I'd love a PR for GnuCOBOL on that as well :-)

Concerning this PR: Do you want to do a follow-up for the MF conditional compilation?

    78  my-define VALUE 5.
      $IF my-define
          DISPLAY 'something'
      $END

@nberth
Copy link
Collaborator Author

nberth commented Apr 8, 2024

Concerning this PR: Do you want to do a follow-up for the MF conditional compilation?

    78  my-define VALUE 5.
      $IF my-define
          DISPLAY 'something'
      $END

The $-variants could be handled in this PR.
The handling of 78-level entries on the pre-processor side will come in another PR, though.

Copy link
Contributor

@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.

There are some issues on the MF side (see comments), but I'm mostly concerned about ISO.
I don't know if there's a preparser applied or if the compdir_grammar.mly keywords are taken "verbatim".

ISO says:

A compiler directive is composed of the compiler directive indicator >>, optionally followed by the COBOL character space, followed by compiler-instruction. The compiler directive indicator shall be treated as though it were followed by a space if no space is specified after the indicator.

Also note that "the COBOL character space" can consist of multiple character spaces.

I do wonder: does

>> IF something DEFINED
    PERFORM something
  >>  ELSE
    PERFORM other-stuff
   >>END-IF

works.

src/lsp/cobol_preproc/compdir_grammar.mly Outdated Show resolved Hide resolved
src/lsp/cobol_preproc/compdir_grammar.mly Outdated Show resolved Hide resolved
src/lsp/cobol_preproc/compdir_grammar.mly Outdated Show resolved Hide resolved
src/lsp/cobol_preproc/compdir_grammar.mly Outdated Show resolved Hide resolved
…ple spaces

As per the ISO/IEC 1989:2014, page 92:
> Anywhere a space is used as a separator or as part of a separator,
> more than one space may be used.
@nberth
Copy link
Collaborator Author

nberth commented Apr 9, 2024

There are some issues on the MF side (see comments), but I'm mostly concerned about ISO. I don't know if there's a preparser applied or if the compdir_grammar.mly keywords are taken "verbatim".

Those >> and $-prefixed keywords are recognized during a lexing phase that occurs before, and interveaning spaces are removed at that stage.

ISO says:

A compiler directive is composed of the compiler directive indicator >>, optionally followed by the COBOL character space, followed by compiler-instruction. The compiler directive indicator shall be treated as though it were followed by a space if no space is specified after the indicator.

Also note that "the COBOL character space" can consist of multiple character spaces.

I do wonder: does

>> IF something DEFINED
    PERFORM something
  >>  ELSE
    PERFORM other-stuff
   >>END-IF

works.

For that, I originally followed GnuCOBOL. The version I used did not allow more than one character, so >> ELSE might not be recognized. I think that's an easy PR — but maybe that was fixed recently.

@GitMensch
Copy link
Contributor

For that, I originally followed GnuCOBOL. The version I used did not allow more than one character, so >> ELSE might not be recognized. I think that's an easy PR — but maybe that was fixed recently.

Oh, please check that then.

Concerning this PR it looks good to me now.

@nberth nberth merged commit 0bc1ba2 into OCamlPro:master Apr 9, 2024
4 checks passed
@nberth nberth deleted the more-preproc-directives branch April 9, 2024 08:49
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