Make Options and Out thread-safe. #258

regisd · 2017-11-11T13:53:38Z

Split Main in 2 classes
- Main the cli client
- LexGenerator that generates java code
- The later is used by the jflex-maven-plugin, the ant task and the GUI, i.e. Main.generate(lexFile); becomes new LexGenerator(generatorOptions).generate(lexFile)
Make Options an immutable object
Remove all statics from Out
- Use System.out for DEBUG statements
- Throws exceptions when possible (which removes a few dependencies on this static Out logger)

The goal is to make the generator thread-safe. #214

- Split Main in 2 classes - Main the cli client - LexGenerator that generates java code - Replace manual command line parsing by commons-cli - Replace Options by GeneratorOptions, an immutable object - Remove all statics from Out - Deleted dead DEBUG statements - Throws exceptions when possible (which removes a few dependencies on this Out logger) - Also simplify error formatting with varaargs - Update maven-jflex-plugin - To reduce scope on this XL change: - Move anttask in a separate directory. Build broken, even though mostly update. TODO: Make it compile as a POM. - Move GUI in a separate directory. Build broken. TODO: Make it compile as a POM. - testsuite-maven-plugin build broken. Renamed Main to Tester to avoid confusion with jflex.Main.

regisd · 2017-11-11T13:55:06Z

This is only for early code review. I know that all POM modules don't compile and that tests probably don't pass.

lsf37

Thanks for taking on that project, it looks like it's quite sizeable (makes sense, it threads through everything).

I think the approach is mostly good, main issues I can see are:

avoiding external dependencies unless there really is a substantial benefit
DEBUG should remain (it's not pretty, but it's not as dead as it looks, I'm still using it)
this PR is mixed in with the CLI PR (see comments there). Of course they depend on each other, but I'd still try to separate the new Options handling from the CLI parsing.

lsf37 · 2017-11-11T22:46:08Z

jflex/src/main/java/jflex/CharClasses.java

@@ -120,11 +120,6 @@ public int getNumClasses() {
  public void makeClass(IntCharSet set, boolean caseless) {
    if (caseless) set = set.getCaseless(scanner.getUnicodeProperties());

-    if (DEBUG) {


We can't remove DEBUG, it's not dead or unused, it's just a static DEBUG option that I use when trying to get a trace of what's really going on in the algorithmic part. It's slightly horrible println debugging, but IDE debugging takes way too long to set up for this.

It's Ok for DEBUG to remain static, since it's only for developing.

Right. Could the debugging be done on System.out rather than Out?

I was a bit unsure of the responsibiilty of Out. But because it's seems to have a state of the output (a count of warnings), it's not thread safe.

Yes, that's a good point. I don't think it would be a problem to do the debug output on System.out instead.

jflex/src/main/java/jflex/StatePairList.java

lsf37 · 2017-11-11T22:54:15Z

jflex/src/main/java/jflex/GeneratorOptions.java

+package jflex;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.base.Optional;


I'm not keen on introducing additional dependencies. The thing we're trying to achieve here seems relatively simple and pretty much the same as in the non-static setting. Do we gain much by using a builder pattern here instead of just a plain object with constructor for defaults?

AutoValue is only an annotation with compile-time dependency. In the end, this automated approach will only bring one (generated) class in the jar. I think its brings enough value (cleaner definition compared to getters and setters ; immmutable object by nature ; free toString and free equals) that's I want to convince you it's worth it.

On the other hand, guava is a huge beast. I didn't think about the cost, because I'm used to use it. It's probably not woth it just for Optional.

I don't actually find it cleaner ;-) It probably is cleaner when you are used to the framework, because then it's a standardised pattern, but if you're not used to it, it's another largish library to look at.

On the dependency point, separately, because I feel somewhat strongly about that:

Introducing additional dependencies can have a rather large cost, it depends on how large the project is and what the dependencies are doing. Mostly, these are:

you need to understand the dependency/library you are using

you need to deal with the additional complexity of managing them. mvn does that well, but that's only one of many build methods we are trying to support.

users (developers) need to track and install them as well

the dependencies will develop and you have to maintain compatibility

We've so far managed to have the cup runtime as the only dependency for JFlex, which is very nice.

Of course there are many advantages as well, esp if you are using those dependencies a lot or know them well anyway. It's a trade-off. The simple kind of CLI parsing and options would be worth a dependency if we had a set of projects that all do the same thing. For just JFlex, that code is so simple that I find it much easier to avoid the build time messiness and the need to keep track of what an external library is doing.

lsf37 · 2017-11-11T22:55:10Z

jflex/src/main/java/jflex/IntCharSet.java

@@ -23,8 +23,6 @@
 */
 public final class IntCharSet {

-  private static final boolean DEBUG = false;


These are not dead, should stay in and are fine to remain static.

lsf37 · 2017-11-11T22:59:52Z

jflex/src/main/java/jflex/Macros.java

@@ -44,15 +44,6 @@ public Macros() {
   * @return <code>true</code>, iff the macro name has not been stored before.
   */
  public boolean insert(String name, RegExp definition) {
-
-    if (Options.DEBUG)


Should remain. It's Ok to move it out of the Options class, but there should be a global static DEBUG flag somewhere that generates a detailed trace when needed (I was actually about to use this for looking at the issue about macros in char classes).

lsf37 · 2017-11-11T23:00:37Z

jflex/src/main/java/jflex/Main.java

 import jflex.unicode.UnicodeProperties;
+import org.apache.commons.cli.CommandLine;


This seems mixed with the other PR on changing the CLI.

Yes, I started here when I refactored the (jflex) Options, and then realized that the commons-cli change was unrelated and could be moved in a different channge (now PR #259)

lsf37 · 2017-11-11T23:08:55Z

jflex/src/main/java/jflex/NFA.java

-    if (Options.DEBUG)
-      Out.debug(
-          "Adding nfa for regexp " + regExpNum + " :" + Out.NL + regExps.getRegExp(regExpNum));
+    if (GeneratorOptions.DEBUG)


This is good. I didn't first see that GeneratorOptions.DEBUG is still around. The other parts that used Options.DEBUG should use this one, and the other smaller scope static DEBUGs can just stay in the classes they are (they are separate either because they slow things down a lot or produce huge amounts of output unless you are looking at specific test cases).

This one was easy to keep because the NFA already depends on the Out instance. I'm afraid the others were not, and I didn't want to bring a dep on Out just for a DEBUG statement.

jflex/src/main/java/jflex/Options.java

# Conflicts: # jflex-maven-plugin/src/main/java/de/jflex/plugin/maven/JFlexMojo.java # jflex/pom.xml # testsuite/jflex-testsuite-maven-plugin/src/main/java/jflextest/Exec.java # testsuite/jflex-testsuite-maven-plugin/src/main/java/jflextest/JFlexTestsuiteMojo.java # testsuite/jflex-testsuite-maven-plugin/src/main/java/jflextest/TestCase.java

…multithread

lsf37 · 2018-09-09T22:40:57Z

Sorry for the noise - Im fine with moving this to 1.8.0, just clicked the wrong button!

regisd · 2018-09-18T12:32:05Z

TODO: Test the GUI. I guess I broke it.

Not compiled / not tested

…multithread

sarowe · 2019-01-02T19:24:23Z

The Lucene/Solr build uses JFlex's Ant task to generate multiple scanners, and options from previous invocations in the same Ant session bleed into further invocations. E.g. if I specify a non-default skeleton for one scanner, it will be used for all further invocations that don't specify a skeleton (expecting to use the default skeleton).

I haven't tried the work done here - thanks @regisd for your efforts! - but I just wanted to point out that thread safety is not the only issue here: serialized same-session invocations are also problematic in all released JFlex versions.

regisd · 2019-11-23T20:09:22Z

This is old and there are many merge conflicts. I'm giving up for now.

The Lucene/Solr build uses JFlex's Ant task to generate multiple scanners, and options from previous invocations in the same Ant session bleed into further invocations.

This is not really the behaviour I would expect, but it could be supported by the ant task itself, i.e. the ant task keeps a reference to OptionsBuilder, and updates between invocations.

regisd requested a review from lsf37 November 11, 2017 13:53

lsf37 reviewed Nov 11, 2017

View reviewed changes

regisd self-assigned this Nov 12, 2017

regisd and others added 24 commits November 13, 2017 23:44

Revert Options.DEBUG blocks

d4be759

Revert commons-cli

1575f09

Fix NPE

25e5f67

Merge branch 'master' into multithread

3d96cb2

Fix POM description for cup-maven

52cc713

Fix testsuite

983ea40

Merge branch 'master' into multithread

08e03e8

Move LexGenerator builder outside the try/catch

39977a1

Merge branch 'master' into multithread

2f3dfc2

Remove dep on AutoValue

a9aac43

Don't use optional

82d6a92

Rename GeneratorOptions to Options

6500a41

Update GUI

5287525

Update mojo

288810e

Merge branch 'master' into multithread

8949ed4

Move gui and ant back where theywre

7c2ead3

Merge branch 'master' into multithread

7f9ffa2

delete pom for anttask

f304e61

Merge branch 'multithread' of https://github.com/jflex-de/jflex into …

0c5bc38

…multithread

restore commented code

22c2902

Revert minor changes

e476a01

Merge branch 'master' into multithread

484abcd

Reformat code

d10c773

traviscibot added 2 commits September 16, 2018 22:55

Merge branch 'master' into multithread

e2d362a

Fix default encoding option for Mojo

116c378

regisd force-pushed the multithread branch from 41fe85d to 116c378 Compare September 16, 2018 21:07

regisd added 7 commits September 22, 2018 00:14

Merge branch 'master' into multithread

106b7e6

Merge branch 'master' into multithread

fbb98e8

Use default platform encoding by default

991a64c

Merge branch 'multithread' of github.com:jflex-de/jflex into multithread

cb4fd5b

Merge branch 'master' into multithread

6547eba

Merge branch 'master' into multithread

694646e

fix simple-maven/src/test/java/YylexTest.java

370f7a1

regisd added 6 commits September 30, 2018 22:40

Merge branch 'master' into multithread

d946985

typo

443a8fb

Merge branch 'master' into multithread

cc85a70

Merge branch 'master' into multithread

5eec36a

Merge branch 'master' into multithread

0730438

Not compiled / not tested

Merge branch 'multithread' of https://github.com/jflex-de/jflex into …

50843a3

…multithread

regisd added 3 commits October 21, 2018 08:39

Merge branch 'multithread' of github.com:jflex-de/jflex into multithread

a37349e

Merge branch 'master' into multithread

3260e11

Merge with master

197caa0

regisd closed this Nov 23, 2019

regisd removed this from the 1.8.0 milestone Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Options and Out thread-safe. #258

Make Options and Out thread-safe. #258

regisd commented Nov 11, 2017 •

edited

Loading

regisd commented Nov 11, 2017

lsf37 left a comment

lsf37 Nov 11, 2017

regisd Nov 12, 2017

lsf37 Nov 12, 2017

regisd Nov 13, 2017

lsf37 Nov 11, 2017

regisd Nov 12, 2017

lsf37 Nov 12, 2017

lsf37 Nov 12, 2017

lsf37 Nov 11, 2017

regisd Nov 13, 2017

lsf37 Nov 11, 2017

regisd Nov 13, 2017

lsf37 Nov 11, 2017

regisd Nov 12, 2017

lsf37 Nov 12, 2017

lsf37 Nov 11, 2017

regisd Nov 13, 2017

lsf37 commented Sep 9, 2018

regisd commented Sep 18, 2018

sarowe commented Jan 2, 2019

regisd commented Nov 23, 2019

		import jflex.unicode.UnicodeProperties;
		import org.apache.commons.cli.CommandLine;

Make Options and Out thread-safe. #258

Make Options and Out thread-safe. #258

Conversation

regisd commented Nov 11, 2017 • edited Loading

regisd commented Nov 11, 2017

lsf37 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsf37 commented Sep 9, 2018

regisd commented Sep 18, 2018

sarowe commented Jan 2, 2019

regisd commented Nov 23, 2019

regisd commented Nov 11, 2017 •

edited

Loading