Fix cics converse rewrite #2535
Conversation
Add check for FROMLENGTH and FROMFLENGTH as they are mutually exclusive.
Add subrule parsing and checks, fix tests.
|
@slavek-kucera @chacebot Could you review this when you get the chance? |
Fix checkstyle error.
...in/java/org/eclipse/lsp/cobol/implicitDialects/cics/utility/CICSOptionsCheckBaseUtility.java
Outdated
Show resolved
Hide resolved
server/engine/src/main/antlr4/org/eclipse/lsp/cobol/implicitDialects/cics/CICSParser.g4
Outdated
Show resolved
Hide resolved
| protected <E extends ParserRuleContext> void checkIfSelfCalledMultipleTimes(String options, E ctx) { | ||
| if (ctx.getParent().getRuleContexts(ctx.getClass()).size() > 1) { | ||
| throwException(ErrorSeverity.ERROR, getLocality(ctx), "Options \"" + options + "\" cannot be used more than once in a given command.", ""); | ||
| } | ||
| } | ||
|
|
||
| protected <E extends ParserRuleContext> void checkSubrules(E ctx, Map<Integer, Consumer<ParserRuleContext>> subruleOptions) { | ||
| ArrayList<ParserRuleContext> childRules = new ArrayList<>(ctx.getRuleContexts(ParserRuleContext.class)); | ||
| for (ParserRuleContext child : childRules) { | ||
| if (subruleOptions.containsKey(child.getRuleIndex())) { | ||
| subruleOptions.get(child.getRuleIndex()).accept(child); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this could be much simplified (and eventually integrated with the token duplicate checking).
Something like
protected void checkSubrules(ParserRuleContext ctx, Map<Integer, String> subruleOptions) {
Set<Integer> seenRules = new HashSet<>();
for (ParserRuleContext child : ctx.getRuleContexts(ParserRuleContext.class)) {
int ruleId = child.getRuleIndex();
String name = subruleOptions.get(ruleId);
if (name == null) continue;
if (seenRules.add(ruleId)) continue;
throwException(ErrorSeverity.ERROR, getLocality(child), "Options \"" + name + "\" cannot be used more than once in a given command.", "");
}
}with the subruleOptions being
private final static Map<Integer, String> subruleOptions = new HashMap<Integer, String>() {
{
put(CICSParser.RULE_cics_into, "INTO or SET");
put(CICSParser.RULE_cics_converse_from, "FROMLENGTH or FROMFLENGTH");
put(CICSParser.RULE_cics_converse_to, "TOLENGTH or TOFLENGTH");
put(CICSParser.RULE_cics_maxlength, "MAXLENGTH or MAXFLENGTH");
}
};There was a problem hiding this comment.
That won't work. The intent of the current Map and checkSubrules function is tying the rule index to a pointer to a given function using a Consumer type.
It resolves needing to write out the Switch statement, lessening overhead. You could even have a base set which is then added to on a per main rule basis.
There was a problem hiding this comment.
I think I understand the intent. I am proposing to replace all that with the mechanism described above. All those functions (checkInto, checkFrom, ...) call throwException when they detect duplicates, the only variability is in the name used in the error message.
There was a problem hiding this comment.
Sure for this so far, thing is there will be times we want more flexibility in the functions being called. (ie. Mutually exclusive rules/tokens, change what is checked if a given token is found and so on.)
Switching to this would remove a lot of that flexibility.
There was a problem hiding this comment.
I'd wait for that time to come and then tailor the solution to whatever the problem will actually look like.
There was a problem hiding this comment.
Just renamed it for clarity. Its purpose wasn't to check anything, but to call the corresponding subrule function(s).
There was a problem hiding this comment.
Implemented the rule duplication check into the existing checkDuplicates function in the latest commit.
Renamed checkSubrules to callSubruleFunctions for clarity of purpose.
Add duplicate checks for rules
Removed unused functions
* Add check for FROMLENGTH and FROMFLENGTH Add check for FROMLENGTH and FROMFLENGTH as they are mutually exclusive. * Add subrule parsing and checks Add subrule parsing and checks, fix tests. * Fix checkstyle error. Fix checkstyle error. * Removed unused function * Renamed cics_converse_from/to to fromlength/tolength * Renamed checkSubrules to callSubruleFunctions Renamed checkSubrules to callSubruleFunctions for clarity of purpose. * Add duplicate checks for rules Add duplicate checks for rules * Removed erroneous comment. * Removed unused functions Removed unused functions * Fix checkstyle.
Added ability to parse subrules rather than just a single layer down. Added new tests to account for these changes.
How Has This Been Tested?
Ran and tested existing tests, changed error message(s) to reflect changes,
Checklist: