search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[language,parser] Align MDT/OCL and OMG/OCL grammars #454

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 292112 | | Status | CLOSED FIXED | | Importance | P3 normal | | Reported | Oct 13, 2009 03:18 EDT | | Modified | May 27, 2011 02:47 EDT | | Version | 1.3.0 | | Reporter | Ed Willink |

Description

OMG issues 10439 and 13076 request a clear grammar.

The inadequacies of the current precedence, associativity and reserved word specifications require a new OMG Issue.

This Bugzilla refines the MDT/OCL grammar to align very much more closely with the OCL 2.1 grammar and to make the grammar LALR(1) - yaccable. It is therefore possible to strip the actions, change "->" to "::=" and get the required submission.

The attached patch is the grammar. A separate Bugzilla will refine the lexer grammar for escapes in strings and identifiers.

Major Grammar Enhancement\

The parser grammar is now ready for submission to OMG (after removing actions).

All productions are now spelled exactly as in the OCL 2.1 specification: e.g. operationCallExpCS not OperationCallExpCS.\ Plural productions are named consistently with "parametersCS" rather than the "varaiableDeclarationListCS".\ [no intention of changing the AST class spellings.]

The productions now follow OCL 2.1 very closely eliminateing some sharing to impriove readability.\ Productions are reordered in a form that makes for sensible reading of the submitted grammar.

The grammar is now LALR(1) rather than LALR(2); only needs one token lookahead so it is yaccable.

EssentialOCL reserved words are defined as a subset of the CompleteOCL reserved words.

EssentialOCLKWLexer.g and EssentialOCLLexer.g are split off. All CompleteOCL functionality is moved\ from EssentialOcl.y to OclParser.y. QVTr and QVTc have been successfully realised as derived LALR(1)\ grammars with no grammar bloat from CompleteOCL. Copyright notices from both Essential and Complete\ grammars are copied to generated files.

[Unreserving Bag etc, false etc is just a matter of commenting out some lines.]

Minor Grammar fixes\

ClassifierContextDeclCS.simpleName captures the option to name a ClassifierContext as in the last example in 7.7.7

context c : Company inv:\ c.numberOfEmployees > 50

EnumLiteralExpCS is removed. x::y @pre was not parsed. EnumLiterals are therefore parsed as PropertyCallExp.\ The resulting merge of AbstractOCLAnalyzer.modelPropertyCallExp badly needs refactoring.

OperationCallExpCS.pathName is promoted to FeatureCallExp.pathName so that qualified PropertyCallExps can\ be parsed.

unaryExp now allows negated and complemented let expressions.

The dead operationCS production is removed.

KEYWORD_LITERAL is now used as Adolfo pointed out in Bug 184048 and I thought I had responded to.

An otherKeyword production is available to ease derived grammars.

The Backtracking errors now use a simple ERROR_TOKEN wherever an IDENTIFIER could hav e been and so\ recover nearly everything from just the one error option. This may be improved as I start to work more on the editor.

Minor fixes\

AbstractAnalyzer::makeString no longer gives a space prefix - not detected on JUnit tests, looked odd in a backtracking error message.

AbstractParser::makeErrorToken now includes the last character of an error range in a backtracking error message.

AbstractAnalyzer.initPathNameAst is invoked to set the ast on each SimpleName.

[Patch to follow once Bugzilla number allocated.]

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 13, 2009 03:26

Created attachment 149413 (attachment deleted)\ Patch to make grammar LALR(1)

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 13, 2009 14:52

Revising this, although I'll do it tomorrow, since I have checked that it's not a trivial patch, which again needs some contrast with the new spec, submitted issues, etc.

P.S: API tooling derived errors need to be fixed (@since, filters in LPG generated classes, etc).

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 14, 2009 02:56

Created attachment 149494 (attachment deleted)\ Action-less grammars

You may find this ~~attachment ~~ (attachment deleted)more convenient for reviewing the grammar. It is pretty much the revised text for submission as the new OMG issue.

API filters: of course.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 14, 2009 09:51

Comments:

PropertyCallExpCS -> ::= primaryExpCS '.' pathNameCS '[' argumentsCS ']' isMarkedPreCSopt

rule comes from. After investigating I realized that the rule should be:

AssociationClassCallExpCS ::= primaryExpCS '.' simpleNameCS '[' argumentsCS ']' isMarkedPreCSopt

As a result of the investigation, I have detected the following problem in the new specification:\ [A] FeatureCallExpCS -> OperationCallExpCS\ [B] FeatureCallExpCS -> PropertyCallExpCS\ [C] FeatureCallExpCS -> NavigationCallExpCS

and

[A] NavigationCallExpCS -> PropertyCallExpCS\ [B] NavigationCallExpCS -> AssociationClassCallExpCS

Since the propertyCallExp is a NavigationCallExpCS, the second rule of FeatureCallExp should be removed. If you agree:

FeatureExpCS->OperationCalExpCS\ FeatureExpCS->NavigationCalExpCS

NavigationCalExpCS-> PropertyCallExpCS\ NavigationCalExpCS-> AssociationClassCallExpCS

If you don't want to include so many changes, we could raise new bugzilla and discuss there. However, I need some clarifications about the EnumLiteralExpCS before +1ing.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 14, 2009 14:35

(In reply to comment #4)

Comments:

  • I have detected a problem with the MDT-OCL grammar which could derive in some OMG issues: I was wondering where the

PropertyCallExpCS -> ::= primaryExpCS '.' pathNameCS '[' argumentsCS ']' isMarkedPreCSopt

rule comes from. After investigating I realized that the rule should be:

AssociationClassCallExpCS ::= primaryExpCS '.' simpleNameCS '[' argumentsCS ']' isMarkedPreCSopt

You're right wrt the OCL 2.1 spec. Issue 7341 allowed qualifiedPathNames in many places where previously only simpleNames were allowed. I suspect that the PropertyCallExpCS was an oversight. Need to highlight this in the OMG Issue.

As a result of the investigation, I have detected the following problem in the new specification: [A] FeatureCallExpCS -> OperationCallExpCS [B] FeatureCallExpCS -> PropertyCallExpCS [C] FeatureCallExpCS -> NavigationCallExpCS

and

[A] NavigationCallExpCS -> PropertyCallExpCS [B] NavigationCallExpCS -> AssociationClassCallExpCS

Since the propertyCallExp is a NavigationCallExpCS, the second rule of FeatureCallExp should be removed. If you agree:

  • I'd raise a simple OMG issue.
  • You could take into account the MDT-OCL grammar change commented before (change the incorrect propertyCallExpCS rule to the correct AssociationClassExpCS rule).
  • You could consider changing the MDT-OCL grammar to adopt the future issue's resolution:

CallExpCS -> FeatureExpCS CallExpCS -> LoopExpCS

FeatureExpCS->OperationCalExpCS FeatureExpCS->NavigationCalExpCS

NavigationCalExpCS-> PropertyCallExpCS NavigationCalExpCS-> AssociationClassCallExpCS

\ The double reference to PropertyCallExpCS seems to be there solely to force implementers to work hard to resolve a spurious ambiguity. Once one of the references is removed, NavigationCallExpCS flattens as in the patch. The OMG issue will include revision of existing partial grammar to align with the overall grammar.

  • My main doubts about the patch is the removal of the EnumLiteralExpCS, which I don't really understand. I mean, I understand that a EnumLiteralExp can be parsed by the propertyCallExp, but the propertyCallExp which matches the pathNameCS has always been there for accomplish the use of static properties. Why do we need to remove the EnumLiteralExpCS now ? Is also there an inconsistence in the OCL 2.1 spec ?. Which conclusions have you obtained from this ?.

The problem is that A::b could be an EnumLiteralExpCS or a PropertyCallExpCS, the original MDT/OCL grammar author chose to parse EnumLiteralExpCS and ignore PropertyCallExpCS and resolve the ambiguity in the analyzer. However this ignores A::b @pre which can be a PropertyCallExpCS but not a EnumLiteralExpCS , so the patch changes to parsing PropertyCallExpCS and ignoring EnumLiteralExpCS and once again resolving the ambiguity in the analyzer.

  • The VariableDelarationCS and TupleLiteralPartCS (the old variableCS2) clearly differs from the spec. Do you intend to change this in the spec ? It seems that this is a MDT-OCL exclusive implementation solution to anticipate errors at parsing time, isn't it ?. If so, shouldn't this rule (VariableDeclarationCS) take a different name to have a clear idea that it doesn't correspond to the spec VariavleDeclarationCS rule.

The problem is that A::b could be an EnumLiteralExpCS or a PropertyCallExpCS, the original MDT/OCL grammar author chose to parse EnumLiteralExpCS and ignore PropertyCallExpCS and resolve the ambiguity in the analyzer. However this ignores A::b @pre which can be a PropertyCallExpCS but not a EnumLiteralExpCS , so the patch changes to parsing PropertyCallExpCS and ignoring EnumLiteralExpCS and once again resolving the ambiguity in the analyzer.

  • Essential.g: TupleLiteralPartCS: I think that we shouldn't miss the idea that we may be declaring variables for the tupleLiteralExp. What about using at least, variableNameCS and typedVariableCS:
    • TupleLiteralPartCS ::= variableNameCS '=' OclExpressionCS
    • TupleLiteralPartCS ::= typedVariableCS '=' OclExpressionCS

Yes. That's sensible and works.

  • OCLParser.g: the first defExpressionCS rule, declares a java variable which has its first letter capitalized.

Yes. The wonders of gloabl replaces.\

  • What about to rename the old-fashioned AbstractAnalyzer propertyCallExpCS, modelPropertyCallExpCS, etc in favour of callExpCS, featureCallExpCS, etc. If u don't agree due to backward's compatibility (which we are not considering at grammar's level), what about updating the comments, variable's name and such ?

We could do that. Believe it or not I try not to do too much in one go. I'm not so sure.

Changing the grammar production name spellings is necessary for OMG compliance.

Raise it as a separate Bugzilla.\

  • Shouldn't we have any migration guide to help clients to adopt our change-of-names's convention.

Yes.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 15, 2009 01:12

(In reply to comment #5)

Changing the grammar production name spellings is necessary for OMG compliance.

  • it has a nasty client impact; spotting the operationCallExpCS change in QVTr took awhile Changing the AST class name spellings is possible but not OMG-mandated Changing the Analyzer method name spellings is possible but not OMG-mandated
  • I don't think I want to mess clients around here. A few changes to createXXX shouldn't matter,

Raise it as a separate Bugzilla.

Bug 186769 (forward referrences) requires some Analyzer name changes; perhaps we change all the names to have a declare/define prefix for the two phases in the same way as QVTc and QVTr. Other minor spelling inconsistencies can be handled at the same time.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 15, 2009 14:07

You're right wrt the OCL 2.1 spec. Issue 7341 allowed qualifiedPathNames in many places where previously only simpleNames were allowed. I suspect that the PropertyCallExpCS was an oversight. Need to highlight this in the OMG Issue.

Which OMG issue ? that one which u are going to raise with the new grammar ?. This conclusion came from contrasting the patched MDT-OCL grammar and OMG 2.1 OCL one. I don't have any problem if you want to include in your grammar-related issue any new idea derived from this discussion, instead of raising a separated one by me.

The double reference to PropertyCallExpCS seems to be there solely to force implementers to work hard to resolve a spurious ambiguity. Once one of the references is removed, NavigationCallExpCS flattens as in the patch. The OMG issue will include revision of existing partial grammar to align with the overall grammar.

Again, my comments comes from looking at the new OCL 2.1 . So, do you intend to remove the concept of navigationCallExpCS for the next OCL spec ?. What about navigationCallExp which exists in the abstract syntax ?

The problem is that A::b could be an EnumLiteralExpCS or a PropertyCallExpCS, the original MDT/OCL grammar author chose to parse EnumLiteralExpCS and ignore PropertyCallExpCS and resolve the ambiguity in the analyzer. However this ignores A::b @pre which can be a PropertyCallExpCS but not a EnumLiteralExpCS , so the patch changes to parsing PropertyCallExpCS and ignoring EnumLiteralExpCS and once again resolving the ambiguity in the analyzer.

I see. It's OK to me. However I think we could split the current modelPropertyCallExpCS method, it is so huge.

  • The VariableDelarationCS and TupleLiteralPartCS (the old variableCS2) clearly differs from the spec. Do you intend to change this in the spec ? It seems that this is a MDT-OCL exclusive implementation solution to anticipate errors at parsing time, isn't it ?. If so, shouldn't this rule (VariableDeclarationCS) take a different name to have a clear idea that it doesn't correspond to the spec VariavleDeclarationCS rule.

The problem is that A::b could be an EnumLiteralExpCS or a PropertyCallExpCS, the original MDT/OCL grammar author chose to parse EnumLiteralExpCS and ignore PropertyCallExpCS and resolve the ambiguity in the analyzer. However this ignores A::b @pre which can be a PropertyCallExpCS but not a EnumLiteralExpCS , so the patch changes to parsing PropertyCallExpCS and ignoring EnumLiteralExpCS and once again resolving the ambiguity in the analyzer.

Copy - paste ? :)

We could do that. Believe it or not I try not to do too much in one go. I'm not so sure.

Changing the grammar production name spellings is necessary for OMG compliance.

  • it has a nasty client impact; spotting the operationCallExpCS change in QVTr took awhile Changing the AST class name spellings is possible but not OMG-mandated Changing the Analyzer method name spellings is possible but not OMG-mandated
  • I don't think I want to mess clients around here. A few changes to createXXX shouldn't matter,

Raise it as a separate Bugzilla.

  • Shouldn't we have any migration guide to help clients to adopt our change-of-names's convention.

Yes.

So, I'll +1. Just take all the comments above into account before comitting the patch (Don't forget to enable API tooling, and fix @since, api filters and such).

Good work !!\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 15, 2009 15:54

Changes committed to CVS. Once again a complete submit in one go failed. The larger files required file by file submission.

(In reply to comment #7)

Which OMG issue ? that one which u are going to raise with the new grammar ?. This conclusion came from contrasting the patched MDT-OCL grammar and OMG 2.1 OCL one. I don't have any problem if you want to include in your grammar-related issue any new idea derived from this discussion, instead of raising a separated one by me.

Yes.\

The double reference to PropertyCallExpCS seems to be there solely to force implementers to work hard to resolve a spurious ambiguity. Once one of the references is removed, NavigationCallExpCS flattens as in the patch. The OMG issue will include revision of existing partial grammar to align with the overall grammar.

Again, my comments comes from looking at the new OCL 2.1 . So, do you intend to remove the concept of navigationCallExpCS for the next OCL spec ?. What about navigationCallExp which exists in the abstract syntax ?

Yes\

The problem is that A::b could be an EnumLiteralExpCS or a PropertyCallExpCS, the original MDT/OCL grammar author chose to parse EnumLiteralExpCS and ignore PropertyCallExpCS and resolve the ambiguity in the analyzer. However this ignores A::b @pre which can be a PropertyCallExpCS but not a EnumLiteralExpCS , so the patch changes to parsing PropertyCallExpCS and ignoring EnumLiteralExpCS and once again resolving the ambiguity in the analyzer.

I see. It's OK to me. However I think we could split the current modelPropertyCallExpCS method, it is so huge.

Yes. I made the same observation earlier.\

  • The VariableDelarationCS and TupleLiteralPartCS (the old variableCS2) clearly differs from the spec. Do you intend to change this in the spec ? It seems that this is a MDT-OCL exclusive implementation solution to anticipate errors at parsing time, isn't it ?. If so, shouldn't this rule (VariableDeclarationCS) take a different name to have a clear idea that it doesn't correspond to the spec VariavleDeclarationCS rule.

Copy - paste ? :)

Oops. There is a well-formed constraint that mandates the initExpression. It seems not unreasonable to give the user an "= " required syntactic error.

  • Shouldn't we have any migration guide to help clients to adopt our change-of-names's convention.

Yes.

http://wiki.eclipse.org/MDT/OCL/MDT-OCL_3.0.0_Migration_Guide under construction.

So, I'll +1. Just take all the comments above into account before comitting the patch (Don't forget to enable API tooling, and fix @since, api filters and such).

Done. Oops. I had API checking disabled when I generated the patch.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 17, 2009 07:09

Created attachment 149812 (attachment deleted)\ Grammar with more reserved words

This patch is for my backup and for discussion.

The grammar gets simpler once

self, null, true, Bag, Tuple etc become reserved words, allowed in Essential OCL only following a :: in a pathNameCS.

The grammar now has a typed collection literal as Set(String){}.

A new problem appears:

It is not possible to do ->oclAsType(Set(String)) which is obviously required if Collection extends OclAny. Parsing this without one of oclAsType or Set being reserved is hard.

The grammar therefore adds a new TypeNameLiteralExpCS; yet more for the OMG Issue.

The grammar breaks a couple of unit tests that are easily fixed by an _ prefix on a reserved name.

Feel free to\ +1 if you think we should do this now.\ +0 to wait for a while.\ -1 if you hate it.

eclipse-ocl-bot commented 1 month ago

By Nicolas Rouquette on Oct 22, 2009 00:56

+1.

Up to yesterday, I was using the OCL 2.0 branch of the mdt/ocl plugins and the head of the m2m/qvt plugins.\ Then, I saw some updates of yours under:

Track OCL bugs 184048 287993 292112

I updated my copy of the m2m/qvt.declarative and now things are broken.\ Unless I'm totally confused, it seems that the changes you've committed are intended to work with the OCL 2.1 branch, not the OCL 2.0.

For my work at JPL and at the OMG, I really would prefer using the m2m/qvt.declarative with OCL 2.1 rather than OCL 2.0.\ If this involves a transient of bug fixing, I could help with some fixes.

What is clear to me is that we need to clarify which branches of emf/uml/ocl/qvt.declarative are intended to work together (give or take fixing a few bugs in the way).

Suggestions?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 22, 2009 02:51

Created attachment 150200 (attachment deleted)\ Grammar changes removing iterator keywords

The attached grammar goes further in sorting out names.

No iterator names are defined as parser keywords requiring some careful structuring of the expression grammar to satisfy LALR(1).

The keywords are now classed as reserved (always) words and restricted words, allowed after a :: e.g My::String is ok but My::else is not.

With a clear name policy simpleNameCS can be as in the spec, and productions are presented to make comparison with the OCL spec as easy as possible.

Since the previous changes to match spelling are post M2, I think it would be good to do this too before M3. It should have little further impact on users, but why impact twice?

Do you like the new style?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 22, 2009 03:01

(In reply to comment #10)\ Nicolas: Apologies for the trouble. #9 shows that I'm still working hard on the grammar and associated OMG issues. I failed to catch up on QVTd, which was updated yesterday, perhaps just after you synchronized.

Prompted by a newsgroup query there is now http://wiki.eclipse.org/MDT/OCL/FAQ#OCL_Editor to assist in getting the editor working until releng Update Sites are sorted out and migration to MDT/OCL occurs.

Please review #9 which I think contains everything I plan to change/raise an Issue for.

MDT/OCL and M2M/QVTd do not use any post Eclipse 3.5 facilities, so the latest CVS should work with any post 3.5 Eclipse. The MDT/OCL milestone builds are currently suspect, since releng is struggling with a new build environment.

MDT/OCL is likely to require the latest EMF from M4 onwards, since EMF is adding useful abilities to invoke methods dynamically from OCL.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Oct 22, 2009 12:17

Ed,

Tomorrow I have a deadline. I'll review this and the patches you submitted last weekend during friday's afternoon and/or this weekend.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Oct 30, 2009 11:09

Created attachment 150937 Patch matching OMG Issue 10439

Attached implements Issue 10439. Only one change; typedUninitialisedVariable is flattened to avoid the name shift/reduce conflict for QVTr's templateCS.

AbstractParser now implements the Issue 14357 escape sequences, with the addition of octal escapes to preserve compatibility until the committee has expressed its verdict. Default parsing options continue to warn of "" identifiers and do not interpret escapes in ''.

New tests are needed for \u and \x sequences.

We can change the defaults either with a +1, or when we know how Issue 14357 fares.

It would be good to do the above patch in M3 so that the grammar spelling changes all occur in one milestone.

There is another round of CST cleanup to do (for M4).

CSTNode.startOffser/endOffsert are redundant wrt startToken/endToken.

StringLiteralExpCS now has symbol, stringSymbol, unescapedStringSymbol with the same values.

TypeLiteralExpCS needs to be created to relieve overload pressure on VariableExpCS.

LoopExpCS needs to be able to handle the IteratorExpCS/OperationCallExpCS amobiguity.

This should all make the analyzer simpler.

:notepad_spiral: Bug292112f.patch

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Nov 02, 2009 09:22

Hi Ed,

I would suggest not mixing different issues in one patch, since just the discussion of one issue may block all the patch while the changes about the other issue may be accepted.

About grammar refactor (Issue 10439). In general I like the grammar. Regardless the proposed grammar is accepted by OMG RTF, I think it's a good one for our interests. I agree that this would rather be included in M3 in the same milestone in which the the previous changes will be included. However, due to we are so close to M3 release, a new big refactor could produce an inconvenience to QVTo (other EMP). Let's give Radek/Alex a chance to reply. I suppose there should not be an inconvenience for us waiting a week before committing these changes.

Some Questions/Suggestions

About Issue 14357:

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 02, 2009 14:31

(In reply to comment #15)

I would suggest not mixing different issues in one patch, since just the discussion of one issue may block all the patch while the changes about the other issue may be accepted.

It's a difficult compromsie between too many issues in one patch and a long series of approvals. As we've discovered, it is very difficult to provide step B as a patch until step A is in CVS.

About grammar refactor (Issue 10439). In general I like the grammar. Regardless the proposed grammar is accepted by OMG RTF, I think it's a good one for our interests. I agree that this would rather be included in M3 in the same milestone in which the the previous changes will be included. However, due to we are so close to M3 release, a new big refactor could produce an inconvenience to QVTo (other EMP). Let's give Radek/Alex a chance to reply. I suppose there should not be an inconvenience for us waiting a week before committing these changes.

We can wait a week, but I tend to feel projects that track CVS see two changes anyway. Projects that track milestones see an M3 change anyway; let's make the M3 change complete grammar-wise, since this is the one where LPG error mesasages for spelling changes are not guaranteed. (CST changes give obvious Java problems.)\ \

Some Questions/Suggestions

  • I really like the keyword's simplification and iterator's name removal.
  • What do you mean "restricted keyword"?.

'restricted keyword' is a suggestion in Issue 14583. I'm happy to lose it and make all restricted keywords fully reserved. A restricted keyword (e.g. String) is valid after a ::. i.e. MyClass::String is ok but MyClass::and is not.

  • I think that "self" is clearly missed in the keyword's list (In the spec). Actually, in the section 7.3.3 it's related as a keyword. I haven't seen any issue related to it, have you ?.

Issue 14583.

  • IteratorExpCS A.3.1/A.3.2 is a clear example of why the more flexible (and probably more ambiguous) VariableDeclaratorCS is interesting. From my point of view, adding more fined-grained non-terminals is good when you want anticipate errors during the sintatic anlysis, as it occurs for instance with TupleLiteralPartCS. However, it isn't so good when having to include two rules because of using that more fined-grained non-terminals, instead of using VariableDeclaratorCS. If it were required, I would prefer waiting to analysis phase to resolve the ambiguity in this case. However, it is my opinion, feel free to catch it or not.

Again it's a compromise. For complex syntax I prefer a suoerset grammar so that as you suggest the analyzer' can suggest the better meaning. For simple syntax the parser can steer the user. For tuples, initializers are mandaory so the simple requirement can be imposed.

  • PropertyCallExpCS [C] is defined without any isMarketAtPreCS in the spec. It seems that @pre could also be used for static features. So I guess that an OMG issue should be submitted for this. I haven't found any issue about it, have you ?.

I've not given any real though to property semantics. In your earlier review you spotted that I'd added some pathNameCS/isMarkerPre. I suggestred it was necessary, but it is not necessary so there is now no change until it has been considered.

  • It's a pity that the expressions-related grammar gets so complicated again with those SimpleNameExp and the impliesWith/without let. In spite of having a small explanation in the grammar, I would like to think a little more about it and understand your solution.

I don't like it; it's necessary for LALR(1) rather than LALR(2).

  • There are 4 junit tests which fail. The bug is in typedUninitializedVariableCS, untypedInitializedVariableCS, typedInitializedVariableCS: 'setOffsets(result, result, ...);' should be 'setOffsets(result, name, ...);'

Hmm. They pass for me. Is this an other CVS access difficulty?\

About Issue 14357:

  • I need more time to work about this (the OMG issue and resolution, and MDT-OCL solution). It seems that you are breaking the non-standard convention which allow defining identifier with double quotes. It could be a risky decision, specially when 14357 resolution it's not accepted yet. I think we should wait until the issue is approved in the ballot !.

My intention was to specify MDT/OCL's use of double-quoted identifers (extended by escape sequences). This only breaks non-standard quoted identifiers with backslashes in. Have I mis-specified or you mis-read?

The risk is imposing a distinction between '' and "" when QVTo defines them as the same.

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Nov 03, 2009 02:33

(In reply to comment #15)

Hi Ed,

I would suggest not mixing different issues in one patch, since just the discussion of one issue may block all the patch while the changes about the other issue may be accepted.

I agree. Though it's easier to make a big cumulative patch, it is much more difficult to review it or roll back its parts.

About grammar refactor (Issue 10439). In general I like the grammar. Regardless the proposed grammar is accepted by OMG RTF, I think it's a good one for our interests. I agree that this would rather be included in M3 in the same milestone in which the the previous changes will be included. However, due to we are so close to M3 release, a new big refactor could produce an inconvenience to QVTo (other EMP). Let's give Radek/Alex a chance to reply. I suppose there should not be an inconvenience for us waiting a week before committing these changes.

QVTO still depends on 1.4.0. So we needn't making a pause here.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 03, 2009 02:47

Is that two +1's?

I think it is good for QVTo to use 1.4.0 until perhaps M6, certainly until we have a 3.0.0Mx that works on the Update Site. I will be a little worried if QVTo does not switch to 3.0.0 for Helios.

eclipse-ocl-bot commented 1 month ago

By Radomil Dvorak on Nov 03, 2009 05:01

(In reply to comment #18)

Hi Ed,\ we have migrated to OCL 3.0 in a branch recently, a batch migration in a longer time would be even bigger pain. We want to coordinate with GMF (which extends QVTo) for Helios M3.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Nov 03, 2009 05:16

(In reply to comment #19)

(In reply to comment #18)

Hi Ed, we have migrated to OCL 3.0 in a branch recently, a batch migration in a longer time would be even bigger pain. We want to coordinate with GMF (which extends QVTo) for Helios M3.

Ed,

Please, give me a couple of days to find some time to complete the review.

About the testcases. I don't know why you are able to pass them, but it's clear that the submitted patch contains the said bugs

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Sergey Boyko on Nov 03, 2009 07:16

(In reply to comment #18)\ Hi Ed,

QVTo has migrated to CST patch dated by 2009-10-15 (HEAD currently).

Now we're trying latest "Patch matching OMG Issue 10439". We'll check possible issues with it in a few days.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 03, 2009 17:06

Sergey: be very careful with all OCL productions that you re-use/extend. It is much easier to spot the subtle name spelling changes by checking than debugging.

Adolfo: I agree; the tests should fail; the offsets are wrong. All six launch configurations pass for me, until I rebuild all, Now they fail. Very easy fix as you point out.

I'll wait for feedback. It'll be interesting to hear the QVTo view. There should be no impact on GMF since presumably GMF has no visibility of OCL grammar or CST.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Nov 06, 2009 06:16

  • PropertyCallExpCS [C] is defined without any isMarketAtPreCS in the spec. It seems that @pre could also be used for static features. So I guess that an OMG issue should be submitted for this. I haven't found any issue about it, have you ?.

I've not given any real though to property semantics. In your earlier review you spotted that I'd added some pathNameCS/isMarkerPre. I suggestred it was necessary, but it is not necessary so there is now no change until it has been considered.

I think I had pointed a different problem. Anyway, as you say,it needs thoughts. I would need to work on it and propose a solution instead of making you think about it.\ Let's see what I could do for the next milestone.

  • I need more time to work about this (the OMG issue and resolution, and MDT-OCL solution). It seems that you are breaking the non-standard convention which allow defining identifier with double quotes. It could be a risky decision, specially when 14357 resolution it's not accepted yet. I think we should wait until the issue is approved in the ballot !.

My intention was to specify MDT/OCL's use of double-quoted identifers (extended by escape sequences). This only breaks non-standard quoted identifiers with backslashes in. Have I mis-specified or you mis-read?

Yeah, I mis-readed ;P

The risk is imposing a distinction between '' and "" when QVTo defines them as the same.

This has not ever been a problem, since LPG allows to rewrite the extended grammar files.

Final comments:

Ed, although I really like the work you have done about the grammar refactoring, I have some reservations about +1 it. I'm not sure about prohibiting 1.+(2) since it's clearly allowed in the specification 2.1

Unless Alex and Laurent support this decision, I can't +1. I'm in inclined to introduce some temporal nasty tricks to support 1.+(2):

operationSimpleNameCS -> SimpleNameCS\ operationSimpleNameCS -> InfixOperatorCS\ InfixOperatorCS -> +\ InfixOperatorCS -> - ...

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 06, 2009 08:33

adolfo: I'm just off for the weekend.

If you read my Issue 14357 submission carefully you'll see that OCL has never supported 1.+(2). If we want to temporarily retain it for compatibility, it's easy, one of the last things I removed, when I noticed the disambiguating rule.

reservedWord are indeed unused. They provide a hook for someone who wants to try to use reserved words for something. WE coiuld easily delete them.

eclipse-ocl-bot commented 1 month ago

By Sergey Boyko on Nov 09, 2009 04:17

Hi Ed,

I checked QVTo grammar against your patch. It's really great work you've done about OCL grammar re-factoring! \ There are no significant problems for us just a few minor notes:

  1. I'm voting +1 for returning back postfix notation for operators like +/-/not. So that for example 'not(a)' and 'a.not()' would be both possible.

  2. It'll be more convenient for us to replace 'unSingleQuote()' usage in createStringLiteralExpCS() and extendStringLiteralExpCS() to something neutral (something like 'unQuote()'). \ In order to support double quotes we can override only 'unQuote()' instead of replacing whole create..() methods.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 09, 2009 15:23

It seems that the patch is generally welcome. In the absence of any last minute -1's or please-waits, I will endeavour to commit in approx 24 hours time to be pre-M3. I'll try to attach a revised patch in 12 hours including:

A ParsingOption defaulting to true to support conceptual operation names (e.g. +/-/not...

Better SimpleTypeEnum.KEYWORD etc.

Some validation that IterateExpCS is 'iterate'.

Until it is clear how well my proposed resolutions of Issue 14357 and 14583 are received, I'm reluctant to merge unDoubleQuote and unSingleQuote. Hopefully the OCL 2.3 direction will be clearer in an M5 time frame and we can eliminate backward compatibilities.

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Nov 09, 2009 15:33

(In reply to comment #26)

It seems that the patch is generally welcome. In the absence of any last minute -1's or please-waits, I will endeavour to commit in approx 24 hours time to be pre-M3. I'll try to attach a revised patch in 12 hours including:

Ed, do you want to have these changes in M3? If so I will make a one day shift and produce M3 tomorrow. Please let me know.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 09, 2009 16:06

Alex:

Since QVTo (and QVTd) is happy, I'd prefer not to delay. I misunderstood the 11-Nov M3 MDT date. I thought that was start of build not end.

I can probably commit tonight. I've got the operations going and the JUnits backed up and SimpleTypoeEnum sorted. Just the ProblemOption to go.

I was delaying to give you and Adolfo a final chance to veto. Since you're not unhappy and Adolfo's +0.5 reservations are resolved I think we're ok to go. I'll commit as soon as possible.

eclipse-ocl-bot commented 1 month ago

By Alexander Igdalov on Nov 09, 2009 16:27

(In reply to comment #28)

Alex:

Since QVTo (and QVTd) is happy, I'd prefer not to delay.

Agreed. I will build OCL M3 tomorrow evening.

I misunderstood the 11-Nov M3 MDT date. I thought that was start of build not end.

In fact, 11-Nov is after the deadline for us. We are producing milestones on a fixed schedule which you can see at http://wiki.eclipse.org/Helios . Please note that MDT OCL is +1 to the platform, i.e. M3 must be published by 9-Nov 9 am EST. 11-Nov is M3 +3 (the deadline for Uml2Tools, for instance). It's not a big trouble to be late but generally we should try to get aligned with the plan so that we do not make inconveniences to the downstream projects.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 09, 2009 17:26

Committed to CVS HEAD.

Validating 'iterate' will have to wait till validator is enhanced.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Nov 10, 2009 05:04

Hi Ed,

It looks nice ;). Good work !!

I think it's a good point to take up again the LPG2 migration. I'll probably start from the beginning since merging the old-fashioned grammar, which resides in the branch, with this one should probably be like playing in the hell.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 10, 2009 05:26

Thanks all for the favourable opinions.

I found converting grammars with a few regexp replaces quite easy. Definitely don't migrate/. EssentialOCLKWLexer etc are also new.

Bug 294698 is trivial. If you can review it quickly it could make M3, which would motivate me to get a solid QVTd M3 release.

eclipse-ocl-bot commented 1 month ago

By Sergey Boyko on Nov 12, 2009 03:43

Hi Ed,

A few notes on latest patch:

  1. Looks like 'EssentialOCL.g' defines superfluous rule\ CollectionLiteralExpCS ::= collectionTypeCS '{' CollectionLiteralPartsCSopt '}'

    The only rule defined in spec for 'CollectionLiteralExpCS' is\ CollectionLiteralExpCS ::= CollectionTypeIdentifierCS ‘{‘ CollectionLiteralPartsCS? ‘}’

    As the result of the rule mentioned the following incorrect expression\ OrderedSet(Integer) {}\ or even garbage \ OrderedSet(blabla) {}\ are parsed as correct.

  2. Problem reporting in AbstractOCLParser::extendStringLiteralExpCS() should pass 'token' as the last parameter for getEnvironment().problem(..) call (in place of 'joinedString'). Otherwise problem position is undefined.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 12, 2009 04:40

Set(Class){} is intentional. It is the proposed resolution for Issue 12953.

Bug 184329 has discussion of this topic.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Nov 14, 2009 13:54

(In reply to comment #25)

  1. It'll be more convenient for us to replace 'unSingleQuote()' usage in createStringLiteralExpCS() and extendStringLiteralExpCS() to something neutral (something like 'unQuote()'). In order to support double quotes we can override only 'unQuote()' instead of replacing whole create..() methods.

As I suspected; Issue 14357 is likely to be refined. Changing from "xxx" to _'xxx' seems popular, which will mean that OCL will have only one form of quote to unQuote(). The current "" support will be deprecated and so continue to give the old warnings and will not have escape sequence support.

Bug 295166 has been raised for further activity on _ name prefixes.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 27, 2011 02:47

Closing after over 18 months in resolved state.