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

Don't infer package name from lex rule names. #414

Merged
merged 3 commits into from
Oct 7, 2018

Conversation

regisd
Copy link
Member

@regisd regisd commented Oct 6, 2018

Fix #104

Also, make ClassInfo immutable.

@regisd regisd self-assigned this Oct 6, 2018
@regisd regisd requested a review from lsf37 October 6, 2018 21:43
@regisd regisd added the bug Not working as intended label Oct 6, 2018
@regisd regisd added this to the 1.7.1 milestone Oct 6, 2018
@lsf37
Copy link
Member

lsf37 commented Oct 7, 2018

This will still fail if the string package is mentioned on the same line as a semicolon in later sections of the lexer spec.

Test case:

  @Test
  public void guessPackageAndClass_givenClass_package_in_rule() throws Exception {
    String lex =
        "\n"
            + "%%\n"
            + "\n"
            + "%public\n"
            + "%class Foo\n"
            + "\n"
            + "%apiprivate\n"
            + "%int\n"
            + "\n"
            + "%%\n"
            + "\n"
            + "package X  { /* no action; */ }"
            + "\n"
            + "[^]  { /* no action */ }\n";
    assertThat(guessPackageAndClass(lex)).isEqualTo(new ClassInfo("Foo", null));
  }

The code should probably become more aware in which section it currently is, and scan for package name only up until the first %% and for class name only after the first %% until the second %%.

@regisd
Copy link
Member Author

regisd commented Oct 7, 2018

This will still fail if the string package is mentioned on the same line as a semicolon in later sections of the lexer spec.

Test case:

        + "\n"
        + "package X  { /* no action; */ }"

The code should probably become more aware in which section it currently is, and scan for package name only up until the first %% and for class name only after the first %% until the second %%.

You are absolutely right. There are 2 reasons why I dont' want to go for a proper fix:

  • This only happens if the package is not defined in the Java user code ; which is never the case for production code ; i.e. we are only fixing examples.
  • Changes may very well be confllcting with my refactoring of Out and Options.

In the end, there are only three robusts solutions:

  • Only give the output directory ; but this breaks a lot of abstraction. I think the Emitter shuold only have an OutputStream (which is convenient for tests)
  • Let the lexer do two passes.
  • Output the java code in a temp file, and move it at the end of the generation, once the emitter can tell exactly what package it is

@regisd
Copy link
Member Author

regisd commented Oct 7, 2018

I'll submit this for now.

@regisd regisd merged commit 5c7aa19 into jflex-de:master Oct 7, 2018
@regisd regisd deleted the strange-name branch October 7, 2018 12:40
regisd pushed a commit that referenced this pull request Oct 7, 2018
Initial commit 5c7aa19
Author: Régis Décamps <regisd@google.com>
Date:   Sun Oct 7 14:40:34 2018 +0200

    Better infer package name from lex spec (#414)

    * Don't infer package name from lex rule names.

    Fix #104

    Also, make ClassInfo immutable.
@lsf37
Copy link
Member

lsf37 commented Oct 7, 2018

Yes, that's alright. I'll make a low-priority issue so I don't forget coming back to this once you've merged the Out/Option refactor. (I think I can fix this properly reasonably easily).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Generating java class in strange folder [sf#102]
2 participants