first set of changes #4

Hassenforder · 2022-06-05T07:30:27Z

Hello,
It take me a lot of time (I clone the repo unstead fork it, so I lost all commit in my local repo).
Sorry but here you should have a first contribution with some small enhancements
+no-exit to prevent System.exit(),

better messages,
generation of a zip with runtime (easier to integerate in a project as .java files will be compiled by a right compiler,
class dedicated to Timing,
errorManager now filters redundant errors,
some Classes are capitalized
package option to integrate in the cup file the package (destdir forgets the package of the path, it is automatically added by emit)
options are all in options not in emit and in options
dumps are also done by the meit class
may be other changes, sorry for the big pull request but I will produce smallers next time

cheers

jhoenicke

I like the change of having separate Option class and getting rid of some global fields.

In some areas I would like to get rid of even more static things, e.g. the ErrorManager should not be static. However, this can/should be a separate change.

If we change the indentation style, we should do this for all files, preferably in a single commit that only changes the indentation.

While we're at style issues. There should be no space between method name and opening parenthesis. Some of your changes introduce such a space.

jhoenicke · 2022-06-06T10:10:14Z

build.properties

               .
 output.. = bin/
-jars.extra.classpath = ant.jar
+jars.extra.classpath = ../ant.jar


Are these changes intended?

yes/no, in fact I move the ant.jat outside the projet as I have to download it separatly. Unfortunatly, I have to update the build.properties (but may be it is environnement dependant and should not be part of a git.

jhoenicke · 2022-06-07T16:12:17Z

src/com/github/jhoenicke/javacup/ErrorManager.java

+	 *		ERRORLEVEL : MESSAGE
+	 **/
+
+	private void emit (Severity severity, String message, Symbol sym) {


nit-pick: no space before "(".

Sorry I did not know this rule. I will respect it later.

jhoenicke · 2022-06-07T16:14:34Z

src/com/github/jhoenicke/javacup/Main.java

+			if (ErrorManager.getManager().getWarningCount() != options.expect_conflicts) {
+				// conflicts! don't emit code, don't dump tables.
+				options.opt_dump_tables = false;
+			} else { // everything's okay, emit parser.


nit-pick: move comment to next line.

jhoenicke · 2022-06-07T16:28:57Z

src/com/github/jhoenicke/javacup/Main.java

+				emit_erase(grammar);
+
+			/* output the generated code, if # of conflicts permits */
+			if (ErrorManager.getManager().getWarningCount() != options.expect_conflicts) {


Did you change the logic here? This line was getErrorCount() != 0.

yes, before that line we check ErrorCount and (in my next commit FatalCount both 0) to compute tables and at this line only warning (S/R or R/R) can happen. But, in my opinion R/R conflict are not warning but errors as the automatic choice should never be the good one. So later a stronger test should be done like error == 0 and warning == expected. I guess that error should stop and only warning can be expected.

The expected conflicts are checked in builld_parser and an error is reported if it's different. I would not use the warning counter as the counter for expected grammar conflicts; there may be other warnings. The old code should work fine here.

jhoenicke · 2022-06-07T16:39:20Z

src/com/github/jhoenicke/javacup/Main.java

+	private String timestr(String prefix, long time_val, long total_time) {
+		long percent10;
+
+		/* pull out seconds and ms */


We could use String.format here. If you want to avoid floating point, use:

String.format("%s%4d.%03d sec (%d.%01d%%)", prefix, time_val / 1000, time_val % 1000, percent10 / 10, percent10 % 10);

I do not touch this line (too hard to be touch with divide and modulo and much more).

jhoenicke · 2022-06-07T18:27:07Z

src/com/github/jhoenicke/javacup/non_terminal.java

@@ -41,8 +41,27 @@ public non_terminal(String nm, int index)
  /*--- (Access to) Static (Class) Variables ------------------*/
  /*-----------------------------------------------------------*/

+  /*


I think, you can make START_nt a non-static member of Grammar. Only Grammar is using this anyway and every Grammar should have it's own start symbol.

You are completly true ! This afternoon I ran in a nightmare as two consecutive runs, first with errors and second good, I receive an unexpected exception... start_nt was not regenerated and previous first_set was still here. I put a lazy getter and a clear and the field move private to protect the use.

I fact I still leave it static because it is in non_terminal and Grammar just controls when to create it.

I would say it is an error (in the old code) to have a static start symbol in non_terminal. If you have multiple grammars, you cannot share the non-terminal for the start symbol between the grammars, especially as each grammar has a different rule for the start symbol. There is no reason why there should be a single static non-terminal for the start symbol.

I'm leaning towards do the same for the terminals EOF and error, but at least for these they can be the same. Also that needs more care when refactoring to make sure it doesn't break anything.

jhoenicke · 2022-06-07T18:32:48Z

src/com/github/jhoenicke/javacup/ErrorManager.java

+	 **/
+
+	private void emit (Severity severity, String message, Symbol sym) {
+		if (sym != null && sym.value != null) {


Not sure if this is a good idea. What is the intention?

It looks like it stops printing multiple error messages for the same location.

Also since the ErrorManager is static, this would mean that on the second run the errors are no longer printed, doesn't it?

It is a filter on the same symbol. IOn the previous release only the first message is emitted and subsequent messages vanishes. With this filter, a second (third,...) message about the same symbol (should exists and have a value) is not printed. So only the first one is printed but other error about other symbols are printed too.

Sorry, I forgot to reply about the second run. No problem, In Main I call a clear method which nullify the instance so the getter will recreate one fresh with all fields set to initial value.

I think this is a bit dangerous and sym.value may not even mean the right thing. The symbol is really the location in the source file where the error happened and it can be any error like undeclared symbol, symbols declared multiple times, or a unrecoverable syntax error. You may hide some error message here.

If you just don't want to get multiple warnings about undefined symbols, it would be better to do this in the parser and keep a set of symbols that were already warned about there. This way you can be sure that only these kind of double reports are omitted and you get for free the clearing of the set.

jhoenicke · 2022-06-07T18:37:47Z

src/com/github/jhoenicke/javacup/Timer.java

+import java.util.Stack;
+import java.util.TreeMap;
+
+public class Timer {


I like the change of having a timer stack to make the measuring easier. But I'm not sure if it's a good idea to keep all times in a map. Isn't it better to keep the timing information inside the class that performs the timing?

Unless your plan is to make this class handle everything timer related, including formatting the timing information.

Yes, all about time should be here. Usually I prefer have classes to store the information and another class use them according to the context. I am not sure, it is a good idea to embed the 'printing' is the best way. Les say, may be in a part you need time in second and millis and in another part in micros even nano, to solve this you will need several printing method.

jhoenicke · 2022-06-07T18:48:14Z

src/com/github/jhoenicke/javacup/production.java

-      for (int i = 0; i < rhs_length(); i++)
-	result.append(rhs(i).the_symbol.name()).append(" ");
-
+      for (int i = 0; i < rhs_length(); i++) {


Is this change still necessary after fixing the issue with START_nt?

I cannot recognize my change. What I can say is that the following lines can have troubles too. But I fixed it this only this afternoon and cannot be part of the previous PR.
I guess that some PR refer previous lines now moved but the display I have here are with the actual content not the content in the PR.

jhoenicke · 2022-06-07T18:57:44Z

src/com/github/jhoenicke/javacup/Grammar.java

@@ -44,10 +44,10 @@ public class Grammar {
  /* . . . . . . . . . . . . . . . . . . . . . . . . . */

  /** Resulting parse action table. */
-  private parse_action_table action_table;
+  public parse_action_table action_table;


add a getter for these?

BTW, why did you move the dump functions to emit? I see emit more like the class that generates the java code.
Dumping the tables for debugging could stay a grammar function.

To put all writing in one class. So we write code with emit and we dump it with emit. But yes we should have a Dumper class to centralize all dumps.

Hassenforder · 2022-06-07T20:20:23Z

I do not know what the best now. I have some commits in my fork and may be it time to reformat the style and rename the classes before continue ?
My eclipse plugins seems working but I have to check the behaviour with several runs and errors/warnings/ etc. To be sure that it works well.

jhoenicke · 2022-06-07T21:21:18Z

Can you make another branch on which you just do automatic reformatting (you did this automatically, right)? Preferably only for the files you reformatted in this pull request.

This should help to make a diff that only contains the functional changes.

Grammar each time a grammar is created

Hassenforder · 2022-06-11T05:22:24Z

Hello,
I am not sure but it seems that my first commit contains initially 1 commit but now it contains my three commits from the branch.
May be it is ok ?
Basically I have three other PR in the pipeline (one about changes on the class Name to adopt Java class Name, one about styling, and one about member naming) Next will be about javadoc but the cost is high. And I do not know how to continue from now : did I pull them accoring to the current master or did I wait ?

michel-hassenforder added 2 commits June 4, 2022 21:01

first set of changes

66f891a

removed an error on several runs with the same JRE

0fe94f8

jhoenicke requested changes Jun 7, 2022

View reviewed changes

removes the static START_nt from non_terminal and create a fresh one in

329cad1

Grammar each time a grammar is created

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first set of changes #4

first set of changes #4

Hassenforder commented Jun 5, 2022

jhoenicke left a comment

jhoenicke Jun 6, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022 •

edited

Loading

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

jhoenicke Jun 7, 2022

Hassenforder Jun 7, 2022

Hassenforder commented Jun 7, 2022

jhoenicke commented Jun 7, 2022 •

edited

Loading

Hassenforder commented Jun 11, 2022

first set of changes #4

Are you sure you want to change the base?

first set of changes #4

Conversation

Hassenforder commented Jun 5, 2022

jhoenicke 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

jhoenicke Jun 7, 2022 • edited Loading

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

Hassenforder commented Jun 7, 2022

jhoenicke commented Jun 7, 2022 • edited Loading

Hassenforder commented Jun 11, 2022

jhoenicke Jun 7, 2022 •

edited

Loading

jhoenicke commented Jun 7, 2022 •

edited

Loading