search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[parser] Modification to support IMP and exploit LPG 2 #307

Closed eclipse-ocl-bot closed 2 months ago

eclipse-ocl-bot commented 2 months ago

| --- | --- | | Bugzilla Link | 242153 | | Status | CLOSED FIXED | | Importance | P3 enhancement | | Reported | Jul 26, 2008 14:31 EDT | | Modified | May 27, 2011 02:55 EDT | | Version | 1.2.0 | | Depends on | 288666, 298542, 298562 | | Blocks | 297966, 298634 | | Reporter | Ed Willink |

Description

The Eclipse IMP project provides useful automation for editors,\ especially those using LPG. This could be very useful for OCL-based\ languiages such as QVT.

I therefore started modifying org.eclipse.ocl to use LPG 2.x rather\ than LPG 1.x so that IMP could be more naturally exploited.

Eventually I backed out nearly all my changes since they are not\ necessary and create more problems than they solve. A summary of\ these activities is provided below\ -----------------------------------------------------------------\ IMP provides a builder so *.g files are automatically built.\ Seems like a good idea, but only when there are no errors. If\ there are problems, the OCLParsersym.java is set to bad content\ and it's difficult to know which grammar is bad. I disabled the\ builder quite rapidly.

IMP provides an LPG editor, which works well for syntax colouring\ and outlines but has some strange ideas about where the errors are.

IMP (the Ganymede synchronised release) has a bad bug whereby any\ edit of a file with an error hogs CPU. A bit tedious when LPG\ files have spurious errors.

LPG 2 uses lpg.runtime rather than lpg.lpgjavaruntime so any\ change introduces a binary incompatibility. lpg.runtime is not\ in Orbit.

IMP exploits LPG 2 parsers and lexers through very small ILexer\ and IParser interfaces. It would be good for these to be directly\ supported by OCLParser and OCLLexer, however IMP is an incubation\ project, so introducing an MDT-OCL dependency on IMP is undesirable.

LPG 2 deprecates $ so e.g. $empty needs changing to %Empty.\ Unfortunately preventing '$' being converted to '' defeated\ me in the KWLexerMap, so I had to change the escape to # rather\ than $. This mostly worked, but for unexplained reasons the NewCase\ macro for EssentialOCL.gi got a BeginJava/EndJava wrtapped around it.\ Changing all BeginJava/EndJava to BeginCode/EndCode cured the problem.\ Similar macros not in an include did not have this problem.

LPG 2 seems to have an ordering problem with include paths. I had to change\ the %Import section to provide an explicit path. This may be because\ LPG 2 has the IMP paths built-in.

LPG2 appears to introduce bison-style production type suffixes\ such as unary$$OCLExpressionCS ::= ...\ This didn't work for me with either $$ or ##.\ ------------------------------------------------------------------\ IMP also supports non-LPG grammars, so I investigated use of the\ existing LPG 1 grammars as non-LPG IMP grammars. Very little problem,\ just need to clone IMP's SimpleLPGParseController.

Consequently, I recommend that MDT OCL delay migrating to LPG 2\ until LPG 2 and IMP are more mature.

The only significant changes I found worth making to OCL were:\ -------------\ Change CSTNode.startOffset:EInt to CSTNode.startToken:IToken\ Change CSTNode.endOffset:EInt to CSTNode.endToken:IToken\ Add CSTNode.getStartOffset() for compatibility\ Add CSTNode.getEndOffset() for compatibility

Modify all setOffsets() to use setXxxToken rather than setXxxOffset.

Only the dotArrowExpCS ::= NUMERIC_OPERATION '(' argumentsCSopt ')'\ causes trouble through use of setXxxOffset in a non-IToken fashion.\ Perhaps this partitioned IToken usage deserves revisiting anyway.\ -------------\ Add to AbstractLexer \ public abstract int [] getKeywordKinds();

so that IMP can access the list of tokens for colouring.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Jul 28, 2008 04:00

Great work !!!

Having the possibility of (auto-)generating editors and related features seems to be a good step for OCL(-Tools?¿), and extending languages such as QVT, even if we don't take advantages of the possible new features which LPG 2.x could provide.

I'll start to make some experiments around this topic. Thank you very much Ed for this helpful insight ;)

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Jul 28, 2008 11:14

Adding Robert Fuhrer in the hope that the 'experience report' can be appropriately triaged as IMP bugs, LPG 2 bugs, user stupidity.

eclipse-ocl-bot commented 2 months ago

By Christian Damus on Jul 30, 2008 15:13

Adding to the 2.0 plan, assigning to Ed to contribute when the LPG/IMP problems are resolved.

It seems that LPG 2.0 is not API-compatible with 1.x, as the package name is different. So, this bug would have to target the 2.0 release. I'd like to avoid breaking API changes, unless the OCL RTF can release a 2.1 rev this year that fixes some major spec problems (which will necessarily be API-breaking).

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 05, 2008 04:01

To avoid a breaking API change, I am currently realising CSTNode.end/startToken by attaching an eAdapter to the CSTNode. This is a bit klunky and inefficient, but allows Ganymede MDT OCL to be used as-is.

It's just necessary for an XXXAnalyzer to construct a DerivedXxxParser that overrides each setOffset to delegate to the CSTNodeTokenAdapter whose static helper functionality orchestrates the dynamic adapter.

dotArrowExpCS ::= NUMERIC_OPERATION '(' argumentsCSopt ')'

is still a problem though.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 02, 2008 03:45

I am maintaining a patch against 1.2.0 for this enhancement at\ /cvsroot/modeling org.eclipse.m2m/org.eclipse.qvt.declarative/plugins\ /org.eclipse.qvt.declarative.parser/model/OCLCST.patch

In addition to CSTNode.startToken, CSTNode.endToken, I have added CSTNode.ast, the inverse of the initASTMapping().

The simple fix for bug 243976 is also included.

The patch changes the version to 1.2.90.qualifier, which reflects its API compatibility with 1.2.0 in all respects except the CST. Normal plugins that\ just demand 1.2.x can use either. Plugins that care about the CST must explicitly specify 1.2.0 or 1.2.90.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 26, 2008 03:17

Created attachment 113562 ManuallyTrimmed Patch wrt Ganymede Source

I found that addition of an astNode in each CSTNode was helpful as well. Otherwise the attached patch is as described earlier. So the API break is:

Lose CSTNode.startOffset/endOffset. Add CSTNode.startToken/endToken/astNode.

Detailed error messages regarding Numeric operations may exhibit reduced column number resolution.

Are you able to incorporate this change in Galileo?

When you're ready, I can provide a coherent patch for this bug and bug 243976 against current CVS.

:notepad_spiral: 242153.patch

eclipse-ocl-bot commented 2 months ago

By Christian Damus on Sep 26, 2008 08:46

(In reply to comment #6)

Created an attachment (id=113562) [details] ManuallyTrimmed Patch wrt Ganymede Source

I found that addition of an astNode in each CSTNode was helpful as well. Otherwise the attached patch is as described earlier. So the API break is:

Lose CSTNode.startOffset/endOffset. Add CSTNode.startToken/endToken/astNode.

I think that in this case, as in bug 243976, we can avoid API breakage by retaining the startOffset and endOffset. They can be updated automatically whenever the start and end tokens are set.

I am a little concerned about the IToken data type. It isn't serializable, so I'm not sure how persistence of the CST is going to work.

Detailed error messages regarding Numeric operations may exhibit reduced column number resolution.

If we don't eliminate the offsets, then we don't lose that. :-)

Are you able to incorporate this change in Galileo?

It will be much easier to do that if we go a little out of our way to maintain compatibility.

When you're ready, I can provide a coherent patch for this bug and bug 243976 against current CVS.

Great!

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 26, 2008 13:31

I am a little concerned about the IToken data type. It isn't serializable, so I'm not sure how persistence of the CST is going to work.

Good, if somewhat pedantic, point. No one can have actually successfully\ saved and loaded a CST model anyway.

I'll make startToken, endToken, astNode transient and if serialisation \ is important may be have to keep the bloat to provide something serialisable.

eclipse-ocl-bot commented 2 months ago

By Christian Damus on Sep 26, 2008 16:33

(In reply to comment #8)

I am a little concerned about the IToken data type. It isn't serializable, so I'm not sure how persistence of the CST is going to work.

Good, if somewhat pedantic, point. No one can have actually successfully saved and loaded a CST model anyway.

Well, they could. It would have required a bit of fiddling to find containers for some objects (possibly the Resource, itself). I'm practising my pedanticalness for OMG's benefit. :-)

I'll make startToken, endToken, astNode transient and if serialisation is important may be have to keep the bloat to provide something serialisable.

Thanks.

I see in the patch that we're not looking to adopt LPG 2 at this point, which is good from the API perspective. However, in order to schedule this patch into the 1.3 release, we'll have to separate it from the IMP/LPG 2 mandate of this bug. How about moving it over to bug 243976?

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 28, 2008 03:20

This bug started largely as an experience recommending deferring LPG 2 migration, until LPG 2 responds to the minor problems.

I've got my Ganymede-based code successfully using a latest CVS-based OCL, so I hope to submit the patch for this as part of bug 243976 this afternoon. This should allow us to redistribute a standard OCL 1.3.0Mx for use by early QVT Declarative users on Ganymede.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Jun 23, 2009 12:30

Adolfo: After rereading it appears that the MDT-OCL changes to support IMP were done, but LPG-2 was backed out because I could get $ replaced and include files to work.

If you can convert EssentialOCL.g to EssentialOCL.gi etc, then I think we could now be go for LPG 2 in 2.0.0.

[Trimming startOffset/endOffset is something we could consider when we discuss API rationalisation.]

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Jul 02, 2009 08:38

The keynote at Tools/SC this morning had the not particularly inspiring title of "Language Design for Meta-Programming of Software Composition".

It was really mind blowing. It described rASCAL, which simplistically is a Java extension (integrated in Eclipse) that raises all sorts of concepts to first class. The parse_tree type supports direct definition of grammars so that a grammar can be used to define a parser, a parse tree, used for object construction, object pattern matching and visitor action control. The underlying formalism is SDF which is being used by IMP; there is a strong tie between this and IMP. I think we could be looking at more than a ten-fold LOC reduction for the parser and a two-fold reduction for the analyser.

rASCAL will not reach release quality for Helios, so while it might be a lot of fun to play with its current almost-alpha release, we cannot exploit it yet. Post-Helios, I would hope to replace LPG, get the lexer and CST models auto-defined and parsed, and the CST to AST conversion dramatically simplified.

Consequently, I would discourage expending effort converting to LPG 2 now since we may want to ditch LPG altogether.

[Adolfo: Change the target to 3.0.0 if you agree.]

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Jul 13, 2009 05:34

(In reply to comment #12)

The keynote at Tools/SC this morning had the not particularly inspiring title of "Language Design for Meta-Programming of Software Composition".

It was really mind blowing. It described rASCAL, which simplistically is a Java extension (integrated in Eclipse) that raises all sorts of concepts to first class. The parse_tree type supports direct definition of grammars so that a grammar can be used to define a parser, a parse tree, used for object construction, object pattern matching and visitor action control. The underlying formalism is SDF which is being used by IMP; there is a strong tie between this and IMP. I think we could be looking at more than a ten-fold LOC reduction for the parser and a two-fold reduction for the analyser.

rASCAL will not reach release quality for Helios, so while it might be a lot of fun to play with its current almost-alpha release, we cannot exploit it yet. Post-Helios, I would hope to replace LPG, get the lexer and CST models auto-defined and parsed, and the CST to AST conversion dramatically simplified.

Consequently, I would discourage expending effort converting to LPG 2 now since we may want to ditch LPG altogether.

[Adolfo: Change the target to 3.0.0 if you agree.]

Hi Ed,

I'm still on the idea that the benefits (in performance impact terms) of using the LPG 2 backtracking parser vs the efforts needed to convert the grammars are considerable.

Another point is that if we are finally going to include IMP code for the editors, I guess we will need LPG 2. Are we going to ship both LPG plugins in MDT-OCL 2.0 ?

I'm inclined on investing some hours in making this conversion. Since I have an OCL grammar extension, I could also oversee any problem with the extending languages.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Jul 13, 2009 06:21

Adolfo: I was discouraging not forbidding an upgrade.

If you want to do the upgrade assign the bug to yourself.

It might be appropriate to wait till we have resolved all MDT-OCL 1.4.0/2.0.0/3.0.0 discussions.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Jul 19, 2009 07:38

LPG 2 is not in Orbit.

One of Christian's roles was LPG support for Orbit, and he was planning to add LPG 2 to Orbit.

Adolfo: You might care to join Orbit in order to contribute LPG 2.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Jul 27, 2009 06:53

Well, I know what Orbit is but I'm not sure what I should do.

I'm sending an email to Orbit-dev list to ask about information....

I assign to myself the bug...

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 12, 2009 14:03

Hi all,

I have created a KeywordTemplateD.gi, KwLexerMap.gi, LexerBasicMap.gi, LexerTemplateD.gi, EsentialOCL.gi, OCLKWLexer.gi, OCLLexer.gi and finally a new version of OCLParser.g so that they are successfully swallowed by lpg.exe v 2.0.17 (the last LPG version in sourceforge). However 2 main problems arose:

binaryName ::= binaryIdentifier\ /.$NewCase./

which generates the following erroneous code:

        //\
        // Rule 46:  binaryName ::= binaryIdentifier\
        //\
        case 46: {\
             \
        //\
        // Rule 46:  binaryName ::= binaryIdentifier\
        //\
        case 46:\
      break;\
        }

The explanation is that the /.$NewCase./ is enclosed by the $BeginJava and $EndJava macro expansion. I have been working around these problems all the day (this one and the previous one related to the $ symbol). I have also tried to find any new option to obtain the old behavior, but no luck. One solution I have found for this, is the following:

In this way, the OCLParser switch cases are well generated again. I'm not sure if this solution is liked by the team ;P.

Tomorrow, I'll work on the OCLLexer/Parser/Analyzer to see if they can be easily aligned with the new lpg.runtime (v2.0.17). After that, If I manage to properly run the test-cases, I'll provide a patch with all the changes. I also want to merge the current grammars/templates with the new versions of the Templates and Maps of LPG v2.0.17

P.S: I'm in the committer nomination process for the Orbit project, so that we will able to prepare the required bundle with LPG v2.0.17 if we finally decide to go on it and we solve this bug. Robert, haven't you required to solve this concern for the IMP project?

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 12, 2009 14:29

Also OCLParerErrors.g etc in backtracking.

I don't mind a one-off change provided the change is well defined.

If BeginJava etc change, I would prefer a one-off edit that gives sensible macros probably eliminating any that do thing.

It might be kind/prudent to check your well defined procedure on one of QVTr or QVTc. These may have more significant include path issues.

I'm not that keen on losing the $Id$. This should at least have an LPG bug to track its resolution.

Are you able to make the token types work? e.g. unary$$OCLExpressionCS ::= ...\ which should remove numerous casts.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 13, 2009 11:59

Hi Ed,

I think I have got a first approach patch which at least make the test cases pass using lpg.runtime (LPG v2.0.17). I'll post some comments about it later, but let me write some quick replies to your comments.

(In reply to comment #18)

Also OCLParerErrors.g etc in backtracking.

Thank you for pointing out. I have also changed the backtracking parser stuff.

I don't mind a one-off change provided the change is well defined.

If BeginJava etc change, I would prefer a one-off edit that gives sensible macros probably eliminating any that do thing.

I'm not sure if I understand this. Are you suggesting finding out an alternative one?. Maybe Robert could help here, but he hasn't added any comment to the thread since it has been opened.

It might be kind/prudent to check your well defined procedure on one of QVTr or QVTc. These may have more significant include path issues.

I'll do some checks with my QVTo grammar. I'll also write some guidelines to adopt the new way to extend OCL grammars. We must include them in the wiki so that clients are able to easily adopt these changes.\

I'm not that keen on losing the $Id$. This should at least have an LPG bug to track its resolution.

Agree, let's see if sending a bug gets more results than a forum discussion. No responses to your post in sourceforge :(\

Are you able to make the token types work? e.g. unary$$OCLExpressionCS ::= ... which should remove numerous casts.

I haven't worked around this yet. I hope to include some advantages (including OCL API refactor) from the new grammars tomorrow, hopefully with a new patch.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 13, 2009 12:47

Created attachment 144433 (attachment deleted)\ Patch to adop LPG v2. First approach

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 13, 2009 12:47

The attached patch contains the minimum changes I have done so that MDT-OCL is able to successfully run using LPG v2 runtime. If you want to check it you must:

Apart from all I have said, I must add that I encountered some problems while treating backtracking grammars. There were weird behaviors while including parser.XXXX.gi grammars. In principle some problems where solved after setting some %Define section before the %Include sections. But I still had some problems when generating (for example an OCLKWLexer was generated instead of a BacktrackingOCLKWLexer). In the end, I'll change every %Include section by %import section. Everything properly worked like a charm.

Just to remind it. This patch is a first approach so we must take into account the following:

P.S: I still have to work on the extending languages. I'll finally start with one of the QVT declarative languages. I'll post the experiences, tomorrow.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 13, 2009 17:06

In reply to #19

If BeginJava etc change, I would prefer a one-off edit that gives sensible\ macros probably eliminating any that do thing.

I'm not sure if I understand this. Are you suggesting finding out an\ alternative one?. Maybe Robert could help here, but he hasn't added any comment\ to the thread since it has been opened.

I meant that BeginJava does not appear to have the same semantics as before so it may be better to eliminate an old practice completely and replace it with a new practice to avoid a confusing mish-mash.

In reply to #21

  • lpg.runtime must be integrated into Orbit. with a suitable bundle name.

MDT-OCL is a widely used project. We cannot transiently impose a requirement for a non-Orbit dependency; at least not for existing functionality. This patch must therefore wait till lpg 2 is in orbit. You probably want to start a branch and maybe all the parser API changes follow that branch much more interactively till a merge in perhaps M4.

I'm afraid I've got too much OCL development backed up waiting for approvals to start investigating another avenue, so I will not be able to review this for a while. It's best for you to check out one of the QVTx languages first anyway.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 14, 2009 13:45

(In reply to comment #22)

In reply to #19

I meant that BeginJava does not appear to have the same semantics as before so it may be better to eliminate an old practice completely and replace it with a new practice to avoid a confusing mish-mash.

Hi Ed, It's not a problem of the macro semantics. It's a problem of the generator which expects to have the BeginJava macro, so that if the macro doesn't exist in the action-block, it's implicitly introduced.

I have re-read all the thread (I should have done it, earlier >.<). I tried your solution related to use the BeginCode which substitute the BeginJava one. It works but and the following warnings are thrown in the console: ...org.eclipse.ocl/src/org/eclipse/ocl/parser/EssentialOCL.gi:464:18:464:17:10733:10732: Warning: The macro "$BeginJava" is undefined. ...org.eclipse.ocl/src/org/eclipse/ocl/parser/EssentialOCL.gi:464:28:464:27:10743:10742: Warning: The macro "$EndJava" is undefined.

Which prove that the generator expects the $BeginJava and $EndJava macros to automatically enclose the action-block code. Therefore, we have two options:

Both options would be practical, specially if we have the proper documentation for them. I think that those 2 warnings are painless, so I'm inclined to the first one.

In reply to #21

  • lpg.runtime must be integrated into Orbit. with a suitable bundle name.

MDT-OCL is a widely used project. We cannot transiently impose a requirement for a non-Orbit dependency; at least not for existing functionality. This patch must therefore wait till lpg 2 is in orbit.

I agree that we have to get the bundle included in Orbit before we this bug is committed. However, I'm not solving the bundle inclusion in Orbit until we all agree about adopting LPG 2. That's the reasons about providing this partial patch, so that you can check that LPG 2 generator and runtime may be adopted without (too) pain.

I have also been able to run QVTRelation junit tests using LPG 2. Again the main problems appeared while including other .gi from the grammars. For instance, QVTrParser was not able to include the EssentialOCLErrors.gi grammar file. This time replacing %Include section by %import section was not sufficient. I finally managed to solve the problem providing the include-directories as absolute paths, via arguments in the External-tool configuration:

-include-directory=${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/parser/backtracking};${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/parser};${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/lpg};

I guess that tuning the LPG external tool configurations and adding documentation for OCL grammar extender clients should be sufficient.

You probably want to start a branch and maybe all the parser API changes follow that branch much more interactively till a merge in perhaps M4.

I'm not very friend of branches (I didn't have good results with subversive and our local SVN server some time ago). However, I guess that branches should properly work with CVS so no problem for that. \ In any case, I must disagree about waiting until M4 to do the merge. As OCL developer if the grammars and infrastructure parsing is going to change we must introduce these changes as soon as possible, so that all the development efforts concerning this area is done against the new infrastructure. As OCL-extender client I would specially expect this kind of changes as soon as possible as well.

P.S: I'll attach 2 patches, a new OCL infrastructure patch (just to replace BeginJava, EndJava macros by BeginCode EndCode one) a new Patch concerning QVTDeclarative so that you can check the changes concerning OCL extending languages.\ P.S2: The 18th on August I should officially be Orbit committer. There I would just need some +1 by the team to include LPG v2.0.17 in MDT-OCL 3.0.0 in order to start the process to have lpg v2.0.17 included in Orbit.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 14, 2009 13:49

Created attachment 144559 (attachment deleted)\ New OCL with LPG v2 patch.

This patch substitutes the $BeginJava and $EndJava macros by $BeginCode and $EndCode.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 14, 2009 13:53

Created attachment 144561 Patch to align an OCL Extending Language (QVTr)

The patch including needed changes to QVTr grammar files to adopt LPG v2.0.17 generator/runtime.

:notepad_spiral: ocl242153_try2_b.patch

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 14, 2009 14:03

I don't like warnings so the approach that I happened to adopt in my abandoned attempt is not the right one. I think your solution to BeginJava must be better.

Your solution to LPG invocation for QVTr is not very satisfactory; a separate special launch per project. This needs to be raised as an LPG 2 bug.

If you are very close to getting LPG in Orbit I was being needlessly pessimistic about M4. I also don't like branches; we would have significant cross-merge difficulties. Hopefully Alex will +1 the 3.0.0 Bugzilla now that he's back so that I can commit that and the next part of the JUnit tests, then I can start on some more bits including this.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 17, 2009 06:29

I don't like warnings so the approach that I happened to adopt in my abandoned attempt is not the right one. I think your solution to BeginJava must be better.

I liked yours since we can preserve the original macro behavior (which includes the $symbol_declarations macro call). So if u are unpleased with these warnings, a third option comes up which mixes both solutions:

Your solution to LPG invocation for QVTr is not very satisfactory; a separate special launch per project. This needs to be raised as an LPG 2 bug.

I disagree on part 1. You would only need one launcher for the OCL project, actually I would include the launcher I use for OCL based on resource selection:

Location: ${workspaceloc}/lpg.generator.${target.os}${target.arch}/lpgexe/lpg-${target.os}_${target.arch}\ Working Directoy: ${container_loc}\ Arguments: -include-directory=${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/parser/backtracking};${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/parser};${workspace_loc:/org.eclipse.ocl/src/org/eclipse/ocl/lpg};\ ${resource_loc}

With this configuration and having the lpg.generator.XXXX project in your workspace you could execute lpgexe on every .g/.gi grammar file which needs/extends any OCL .g/.gi grammar file.

If you are very close to getting LPG in Orbit I was being needlessly pessimistic about M4. I also don't like branches; we would have significant cross-merge difficulties. Hopefully Alex will +1 the 3.0.0 Bugzilla now that he's back so that I can commit that and the next part of the JUnit tests, then I can start on some more bits including this.

Well, let's see what do our remaining team mates think about this...

Cheers,\ Adolfo

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 17, 2009 15:41

Third option to BeginJava seems good.

Currently I have a single launcher for LPG:

select a .g file, invoke LPG launcher on current selection.

This allows me to LPG the KWLexer until that works, then the Lexer and finally the Parser; and it works for all projects.

I feel that, once an LPG bugzilla has been fixed, the same functionality should be available.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 18, 2009 12:19

(In reply to comment #28)

Third option to BeginJava seems good.

Currently I have a single launcher for LPG:

select a .g file, invoke LPG launcher on current selection.

This allows me to LPG the KWLexer until that works, then the Lexer and finally the Parser; and it works for all projects.

That's what I refer. Exposing an external tool configuration for the OCL project and any other project which extends OCL grammar files, shouldn't worry us about the need of those arguments for the lpgexe generator.

I feel that, once an LPG bugzilla has been fixed, the same functionality should be available.

That's what I'm not very confident. The feedback from LPG team has always been terribly poor. A help question I posted 2007-03-29, is still pending response... So I'll try to avoid be dependent of a bug fixing.... At this moment, the only thing which is really concerning me is the missing of the $Id$ in the OCLKWLexer java files. So we need some vote for this options (any extra option is welcomed) in which I include some + and -.

1) Stay on LPG v1.1\

2) Change OCLKWlexer.gi %Notice section so that we remove the $Id$. (We can miss them, or we can manually add them before commit).\

3) Remove $ token since it's not needed by OCL concrete syntax.\

I'm inclined on the third option. $ token has probably been imported from a version of the LexerTemplate. If there is no need to have this token in the grammar we could definitely remove it, avoiding the problem on the generated OCLKWLexer.java file.

Ed,Alex, Laurent please add more options and/or vote on this. I have finally been promoted to Orbit committer, so If an option different to 1) wins, I'll start to work on having LPG 2.0.17 in Orbit.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 18, 2009 14:54

+1 for Option 2.

We can live without the CVS info until the LPG bug is fixed.

Option 1 is not necessary. You have successfully overcome all the major problems. The minor ones can be tolerated. You missed commenting that the OCL/QVTc/QVTr editors use of IMP runtime uses LPG 2.

Option 3 is very undesirable. $ is often used in extended contexts. We should perhaps only allow it subject to a ParserOption.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 19, 2009 13:08

(In reply to comment #30)

+1 for Option 2.

We can live without the CVS info until the LPG bug is fixed.

Option 1 is not necessary. You have successfully overcome all the major problems. The minor ones can be tolerated. You missed commenting that the OCL/QVTc/QVTr editors use of IMP runtime uses LPG 2.

Good pooint. If OCL editor needs LPG 2 runtime, we have to solve the library inclusion in Orbit anyway. So I'll be on it ;)

Option 3 is very undesirable. $ is often used in extended contexts. We should perhaps only allow it subject to a ParserOption.

Extenders can always add their needs. However, I don't have any inconvenient of having controlled warnings or manually adding a $Id$ to the generated file.

Let's see if we have more feedback. Meanwhile, I'll go on Orbit's tasks.

Cheers !!!\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Laurent Goubet on Aug 20, 2009 06:12

phew, that's some reading.

I'm not that familiar with LPG. Judging by the comments and options given above, I'd choose 2). I'm not that inclined to use any CVS tag and I do not see them as being useful. The history gives better information :p.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 26, 2009 02:56

Adolfo. I've started looking at this post-3.0.0 increment. To avoid duplication of effort can you resubmit the OCL patch with

a) API filters for every IToken etc change.\ b) @since 3.0 for new parser generation artefacts

The QVTr patch probably survives since QVTd has no baseline.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 26, 2009 03:33

No rush for the @since's

Since Lexprs etc have a fundamental phiolsophical change we can just declare a top level per-class API change; no need to worry about fine grained detail.

Looks good. Changes look straightforward and sensible. I can regenerate the parser. Any simple way of making the lpg.exe reference easier?

I'll +1 when the API filters are in place and I've checked QVTc and QVTr.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 26, 2009 13:34

(In reply to comment #34)

No rush for the @since's

we can just declare a top level per-class API change

Ed, Can I create a new API Filter per class ?. how? I have only been able to create a new API filter per error in the class.

I'm on a refactoring of the templates/classes based on the new LPG API. The LPGs lexer/parser has been refactored so that now the generated Lexer doesn't extend LpgLexStream, it just contains the LpgLexStream (AbstractLexer in our case) as a field. Now doing several parsing should imply calling a method, instead of creating new instances of the lexer,parser, etc. This should increase performance in scenarios where a lot of parsing processes are needed, such as an Editor ;).

I hope to get some pleasant results tomorrow.

About Orbit: I should have said that I passed the official voting process, instead of saying that I'm official committer. Burocratic issues are still pending, so I can't do any commit yet. Meanwhile, I have also "pinged" IMP newsgroup to ask about their position around LPG 2.0.17 in Eclipse. I'm still waiting response.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 27, 2009 17:45

No idea how to do a per-class API filter.

super.getPrsStream().makeAdjunct

at line 22 of LexerBasicMap.go needs to be

super.getIPrsStream().makeAdjunct

to avoid a deporecation warning.

I have the editors working fine for QVTc, QVTr, KM3. Just some minor\ JUnit error message spelling to sort out.

So +1, provided you commit with API filters.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 27, 2009 18:40

(In reply to comment #36)

No idea how to do a per-class API filter.

super.getPrsStream().makeAdjunct

at line 22 of LexerBasicMap.go needs to be

super.getIPrsStream().makeAdjunct

to avoid a deporecation warning.

I have the editors working fine for QVTc, QVTr, KM3. Just some minor JUnit error message spelling to sort out.

So +1, provided you commit with API filters.

I have been working two days on this.

I have reworked grammars, even lexer/parser infrastructure, so that an OCLParser is only a facade to parse the CST, which contains the CST-building methods and an a decoupled instance of a IPrsStream. In a similar way, the OCLLexer contains an instance of ILexStream.

I have tried to recover the concept of the original recommended LPG dtParserTemplate, modifying them to adopt the current OCL . I'll also take a look at the btParserTemplate, so that I can analyze if it provides added value to current technique of macro definitions, which allow us to have the deterministic and backtracking parser version.

I hope to finish the tests tomorrow, and I'll make a separate patch for the OCL Editor and the QVTr or QVTc extension (BTW I have some problems with the last CVS version of QVTd which included some comment tokens, or something like that). I'll also upload a text file which should help language extenders to migrate lexer/parser grammars and infrastructure.

I have mostly tried to keep compatibility backwards. However, after this step, It could be useful analyze the current design about the different concerns in the parsing process such as BasicEnvironment, ProblemHandler, etc. It seems that some of this non-easy-to-understand design comes from some deficiencies in the old LPG runtime infraestructure and some needs about compatibility backwards, which we can reconsider in OCL 3.0.0.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 28, 2009 01:00

The comments for the editors are awaiting a +1 on Bug 286724.

BasicEnvironment certainly needs examination. I had to completely reimplement EcoreEnvironment and UMLEnvironment in order to share abstract functionality for an interchangeable Ecore/UML-based editor. My ICSTEnvironments with Root, Node and File variants needs to go high up.

I'm not clear what problem your reorganisation solves. It just seems to make things different. Even if the organisation is a good idea we should proceed in two stages. The current patch does a good job of migrating from LPG 1 to LPG 2. Then we can see about reorganising.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 28, 2009 11:45

Hi Ed,

Sorry but I have been a little bit busy this morning at work.

(In reply to comment #38)

The comments for the editors are awaiting a +1 on Bug 286724.

Ok, I'll take a look to this, right now.

BasicEnvironment certainly needs examination. I had to completely reimplement EcoreEnvironment and UMLEnvironment in order to share abstract functionality for an interchangeable Ecore/UML-based editor. My ICSTEnvironments with Root, Node and File variants needs to go high up.

I'm not clear what problem your reorganisation solves. It just seems to make things different.

In practice, it should improve performance in scenarios where a repetitive parse is needed. In theory, it's just a better design style, IMO.

Another point is that aligning to LPG templates style implies that future improvements of the templates evolution, should facilitate us to adopt said changes.

Even if the organisation is a good idea we should proceed in two stages. The current patch does a good job of migrating from LPG 1 to LPG 2. Then we can see about reorganising.

In principle, I should agree. However, since a first patch must imply migration for OCL extenders. A second patch could also imply a new migration. If we can simplify in just one step, I think it's better for MDT-OCL and OCL extenders. Besides, the patch it's nearly done, I only have to find out why are a couple of test cases failing, and align the QVTDeclarative grammars for them.

I hope to finish the patch during the afternoon. After that, we can vote on our preferred patch.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 28, 2009 14:04

Two phases is undesirable yes, but they are very different. Phase 1 is a simple lexical change that we are happy with. Phase 2 needs review and will be a different kind of change for extenders. Provided we deliver both phases between M1 and M2 I see no problem at all with two phases.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 28, 2009 14:35

Although I have finished the OCL-patch, I'm afraid that you'll have to see the results next week. I want to prepare an example with the OCL-Extending language such us QVTr or QVTc, and revise all the stuff.

Have a nice weekend.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Aug 28, 2009 14:43

(In reply to comment #40)

Two phases is undesirable yes, but they are very different. Phase 1 is a simple lexical change that we are happy with. Phase 2 needs review and will be a different kind of change for extenders. Provided we deliver both phases between M1 and M2 I see no problem at all with two phases.

Ed, I have been working several days on this, so I want to upload the work anyway. In any case, we should even not commit to CVS the phase 1, since we have to include LPG v2 in Orbit, which is pending on my paperwork revision. Sharon is in holidays until next week, so we will have to wait a little bit.

I guess that we will have some time to revise and discuss about the changes I plan to include in lexer/parser infrastructure.

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Aug 28, 2009 14:52

Ok. You'll have to repeat the .g to .gi conversions when everything's ready. It's very easy so not a big problem.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Sep 01, 2009 13:32

Created attachment 146213 (attachment deleted)\ provided patch

Hi Ed,

I have eventually finished this try. Sorry for the long description but it will be helpful when I create a wiki entry to describe the migration from LPG1 to LPG2.

The new versions of grammars/templates/inclusion are based on:

That said, I'll explain the relevant changes or notes I have taken:

OCLKWLexer related Grammars/Templates/Includes

KeywordTemplateD.g to KeywordTemplateF.gi:

  1. All the new template has been adopted, excepting the removal of the unnecessary %Globals section.

KWLexerMap.g to KWLexerMap.gi:

  1. All the new include has been adopted. Since the %Terminal section is defined here, it will be removed from the OCLKWLexer.gi file.

OCLKWLexer.g to OCLKWLexer.gi

  1. $id$ has been removed from %Notice section to avoid a stupid error in the generated OCLKWLexer.java file.
  2. The %Eof and %Terminal sections have been removed since they are defined in the template and include file.

OCLLexer related Grammars /Templates/Includes

In LPG v2 the runtime API has been refactored so that a generated Lexer doesn't extend LexStream. Instead, it will contain an instance of said class, so that the lexer's API is much simpler. Therefore, we will deal with an AbstractLexer class (which will maintain the environment) and AbstractLexerStream (which will contain the logic related to ILexStream). Unfortunately since the LexStream reports the errors, and we have delegated to our BasicEnviroment's problem handler, the AbstractLexerStream must contain the BasicEnviroment as well.

NB. BasicEnviroment/ProblemHandler design should be re-analyzed.\ NB. Some AbstractLexerStream methods should be analyzed to see if we can remove them. They are kept for compatibility reasons.

LexerTemplateD.g to LexerTemplateF.gi

  1. The generated OCLexer will extend the AbstractLexer, which won't be the LexStream anymore. Therefore, the macro names must be accurated:

    • A new macro $super_lexer_class appears which corresponds to the AbstractLexer, the super class which be extended by the generated lexer. This macro may be redefined by extenders
    • the instance of the LexStream used by the template, it will defined and used by the template and the include file. This macro is not intended to be redefined by extenders

  2. Some functionality which resided in the old LexerTemplateD now resides in the new LexerBasicMapF template.

  3. Several constructors based on some reset methods are included in the new template:

    • This resets methods are the KEY, so re-parsing will be made via this methods.
    • The Reader-based constructor and reset methods has been included.
    • The constructor based on no-input (only an environment as parameter) has been removed. A parse/lexer infraestructure will be always constructed on a given input.

  4. Some methods related to LexStream API has been been removed, such us setInputChars.

  5. Some methods related to AbstractLexer API has been kept, due to compatibility backwards, such us lexToTokens. However this method has been moved to AbstractLexer and reworked to invoke the proper lexer template's method.

LexerBasicMap.g to LexerBasicMapF.gi

  1. The original LexerBasicMapF.gi defines an extension of LpgLexStream. Since we have an AbstractLexerStream a new $super_lexer_stream macro is needed (look at point 5).
  2. Since now we have an internal LpgLexStream class, some constructors methods are added this include file.

OCLexer.g to OCLLexer.gi

  1. It seems that the $action_class macro is not needed. Although it could be removed, I have only added a deprecated comment just for compatibilty reasons.

OCLParser related Grammars/Templates/Includes

As occurred with the Lexer, the AbstractParser doesn't extend PrsStream anymore. Instead, it will contain an instance of said class, so that the parser's API is much simpler. Same approach, explained for the lexer, has been taken for the parser.\ On the other, the previous EssentialOCL.g grammar file includes the original parserTemplate code defined by LPG into the own file. The concept of parserTemplate has been rescued and the last version provided by LPg has been adopted. Actually the refactoring of parser infrastructure comes from the template itself.

dtParserTemplate.F

  1. The definition section defined in Essential.OCL .gi was the original LPG dtParserTemplate.g . So, the section has been removed from there and it has been merged into the new dtParserTemplate.F
  2. It has been slightly modified so that the same template may be used by a deterministic or backtrakcing parser, in the same fashion as it's currently solved via macro definitions by Essential.g grammar file.
  3. Since the new LPG version scopes the block action code with a $BeginJava and $EndJava macro when generating the code, it doesn't allow us to use the useful $newCase macro. Therefore, we will make $BeginJava and $EndJava do nothing and we will create a new $BeginCode and $EndCode macros which will be used instead of $BeginJava and $EndJava.
  4. Only one constructor, which receives an AbstractLexer instance is maintained. Note that the $lexer_stream_class macro has not been renamed to avoid extenders to rename this macro as well.
  5. Some macro definitions (such us $parserCore) and some methods (parseTokensToCST), which were initially defined in Essential.gi have been revised and removed/reworked since they are now considered in somehow in the template and/or AbstractParser.

EssentialOCL.g to EssentialOCL.gi

  1. The definition section has been removed (look at point 13).
  2. %EOF and %Error section has been removed since it is defined in the template.
  3. %Headers section doesn't call $parserCore anymore. The code now resides in the template (look at point 16). Besides, now includes couple of methods which avoid changing grammar action-blocks.\ NB. The action-block code could be reworked in the future so that this methods can be removed. Another point is the problem that may cause extenders if we remove this methods. I think \ that a discussion about what to break and not to break can be done in a different bug.
  4. In section %Rules, The macro $BeginActions doesn't need to be called.
  5. Every call to the $BeginJava or $EndJava macros needs to be replaced by $BeginCode and $EndCode macros (look at point 14).
  6. %Trailers section is removed since it is defined in the template.

Changes to OCLParser.g

  1. The main change is that now, an LPG template will be used, so that a new option will be required by the .g grammar file: %option template=dtParserTemplateF.gi
  2. Some options will be removed since they are included by the template => remove table, error_maps, scopes, margin, programing_language, action-block options.
  3. Now the macros which surround the action blocks code are called as $BeginCode and $EndCode (look at point 14).

OCL API impact \ The main impact of the current lexer/parsing infraestructure is the following:

APIFilters \ Every method which deals with a parameter whose type belongs to the old version of LPG runtime it's considered as a new method, even though it has a previous @since tag. It could be confussing that a method which does already exist has an @since 3.0. However since API point of view makes sense, so I have increased the @since tag version to 3.0

I have done an exception with the generated "public $prs_parser_class getParser() { return dtParser; }" method defined in the dtParserTemplateF.gi file, which can't be promoted to the \ AbstractParser due to that dependency on the macro. Inserting the @since tag in the template is not a good idea neither, since a extender language with a different version number would have an incorrect @since tag. So I finally decided to include an API Filter exception in that method.

Cheers,\ Adolfo

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Sep 01, 2009 13:37

Created attachment 146215 (attachment deleted)\ Patch which exploits new Lexer/Parser/Analyzer infrastructure

This patch have reworked the current QVTDeclarative's common infrastructure and OCLEditor to exploit the new OCL parser infrastructure.

A patch which demostrates how align extending languages is coming soon (tomorrow :) ).

Cheers,\ Adolfo.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Sep 02, 2009 14:26

Created attachment 146307 (attachment deleted)\ New OCL with LPG v2 patch

This patch has been created against the last version of CVS (including bugs solved yesterday).\ All the generated classes have been removed from the patch.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Sep 02, 2009 14:41

Created attachment 146308 Patch to align new API and extenfing languages (QVTr)

The attached patch fixes QVTr grammars/editor and OCL editor, exploiting the new parser infrastructure.

The howto migrate OCL-extending grammars:

Grammar changes in an OCL extending language

The following changes were required to the grammar files of an OCL-extending languages such as QVT Relations, so that they can be properly used by the new LPG v2.0.17 generator.

Changes to QVTrKWLexer

  1. Change the old KeywordTemplateD.g used template by the new KeywordTemplateF.gi
  2. Change the legacy $Include OCLKWLexer.g $End section, by the new %Import OCLKWLexer.gi %End one.
  3. Change any $ symbol which was used to mark a section, by the new the new %symbol, so that, for instance, $Define...$End section turns into %Define...%End

Changes to QVTrLexer

  1. Change the old LexerTemplateD.g used template by the new LexerTemplateF.gi
  2. Change the legacy $Include OCLLexer.g $End section, by the new %Import OCLLexer.gi %End one.
  3. Change any $ symbol which was used to mark a section, by the new the new %symbol, so that, for instance, $Define...$End section turns into %Define...%End

Changes to QVTrParser

  1. Include the new OCL ParserTemplate option => add %options template=dtParserTemplateF.gi
  2. Remove the unnecessary options which are already included in the template: table, error-maps, scopes, margin, programing_language, action-block
  3. Change any use of the $prs_stream_class macro by the macro $super_parser_class
  4. Change any $ symbol which was used to mark a section, by the new the new %symbol, so that, for instance, $Globals...$End section turns into %Define...%End
  5. Change any $empty appearance by %empty.
  6. Change any $BeginJava and $EndJava macro use, by $EndCode and %EndCode

Regardin the point of view of the grammars, the only different point which the migration defers from the previous one (one step) is the point 9. Regarding the point of view of API, the minor changes, which I commented yesterdary, about the new use of creating the parser/lexer and accessing their prsStream/lexStream are required.

P.S: Due to some reasons I haven't caught, the QVTrParser.g file is detected as binary file when creating the patch. So you may encounter problems when applying it, so that u will have to fix some BeginJava- to-BeginCode and EndJava-to-EndCode macros to successfully generate the QVTrParser. P.S: The patch still needs JavaDoc comments, copyrights and probably some tuning. However it's sufficient to start discussion about it. BTW, I have received the Committer status on Orbit one hour ago, so I'll start the commented task, tomorrow :D.

Cheers, Adolfo.

:notepad_spiral: ocl242153_try4_b.patch

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 02, 2009 15:35

(In reply to comment #46)

Created an attachment (id=146307) [details] New OCL with LPG v2 patch

This patch has been created against the last version of CVS (including bugs solved yesterday). All the generated classes have been removed from the patch.

You seem to have removed all the parser grammars too.

The patch just contains some blank line insertions.

eclipse-ocl-bot commented 2 months ago

By Adolfo Sanchez-Barbudo Herrera on Sep 02, 2009 16:34

Created attachment 146322 (attachment deleted)\ New OCL on LPG v2 patch

Oh Dear,

My apologies. I guess this is the good one.

eclipse-ocl-bot commented 2 months ago

By Ed Willink on Sep 03, 2009 02:32

Preliminary comments:

Change the {lpg.exe} in the launches to {lpg2.exe} so that the old LPG cannot be used accidentally.

The test_hebrew_singleQuote_135321 fails in both Ecore and UML bindings.

EssentialOcl.gi has an "Invalid" token which EssentialOcl.g does not.

Lexer.lexer() replaces lexToTokens(). lexToToken was not a verty good name,\ but lexer is differently bad; not a verb, and now requires caller to getIPrsStream. Suggest chanke to lex(AbstractParser) using a verb and hiding the indirection.

Similarly change parseTokensToCST to parse() rather than parser().

AbstractOCLParser has a @SuppressWarnings("nls"). I think this should only be used in auto-generated or test modules. Real code should have per-string NLS apologies, since at one point OCL was internationalised and might well be again.

The revised architecture seems to solve a lot of the uncomfortable compatibilities I had to struggle with originally, but I think it deserves more careful thought to get it right in one go.

There really are two changes here. I do not like reviewing them together. The LPG change involves numerous trivial changes that are tedious and not that necessary to review. The architecture change involves just a few changes that are masked by the volume of trivial changes. Please prepare a first set of patches that is a no-functionality change migration to LPG2. We can commit these (perhaps to a branch to assist creation of a two stage patch) and then review the architecture revision patch.