Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[] Impact Analyzer review

Hi Michael

That's very encouraging as regards functionality. Just need the review.

Because the code is largely the work of a single author, and no other committer has read more than a few lines, I am keen that it should be reviewed. From my own experience I am aware that code evolves and not all the revisions that are desirable actually happen. The IA code has certainly evolved, so I expect that at least a minor clean up is desirable.

By review, I mean, pay attention to (in decreasing priority)

External API
This is very difficult to change once promoted so it wants to be right. My understanding is that there is a very small 'facade' so the external API is small. It's clearly useable, but could or should it be better? Is it too fat or too thin? Is it extensible?

Internal API
This too can be difficult to change if internal APIs are not constrained. How much functionality is exposed to arbitrary derived classes by protected access. Is this necessary? Can it be shut off? Can a minor refactoring support extension more easily?

Can inefficient O(N*N) algorithms be replaced by O(N)? Can O(N) be replaced by O(1)? Are results sensibly cached in variables?

For each field/variable, is the lifetime of the variable compatible with its validity. i.e. is the status appropriately updated in response to a Resource change, and is memory leakage avoided? Resource change is badly handled by OCL at present, so it may be that for now there is a top level Javadoc mandating total reconstruction after a change. One day the IA might monitor resources and automatically update itself.

plugin and other global names
These obviously change. The simplest change is just delete ".examples". Do we want to do something else?
It would be nice if the event plugins went to EMF, but that doesn't look likely, so they too need review.
It would be nice to have names that can accommodate the pivot model sometime. I would like to try to partition the code into the run-time code that performs (re-)evaluation and the meta-run-time code that maintains the control objects that make IA so good. If this is possible, then we want corresponding names. Perhaps
I hope that migration of the run-time code to align with the code generated Java can be done quite easily, since the code generated Java makes no use of any form of the OCL meta-model; just the polymorphic Values and polymorphic Domain model for which there is direct and Reflective Ecore support. Perhaps
Migration of the meta-run-time code will be harder because that obviously makes use of the OCL meta-model. Perhaps
In order to avoid code duplication, code that is independent of Ecore/UML/Pivot should be in perhaps
It may also be appropriate to place some declarations independent of Ecore/UML/Pivot such as extension points in
We cannot easily use org.eclipse.ocl since that is highly Ecore/UML dependent.
NB being independent of Ecore does not prohibit use of EObject, EObject.eClass() etc. I hope that the external API facade can be Ecore/UML/Pivot independent and so in org.eclipse.ocl.common.impact.

Are package, class, method names and to a lesser extent field names consistent and readable?

CodeGen API
The new direct OCL to Java code generator can be invoked automatically from genmodel. Addition of some GenAnnotation could control how an IA instantiation was auto-generated. It would be good if this relatively simple interface could be thought through so that it is easy to implement after Juno, if not implemented for Juno.

There are many JUnit tests. Is the coverage adequate? Are all of the tests really useful? Can the tests be combined into one overall ImpactAnalyzer test suite? Is a memory leakage test needed?

Is the code generally readable? I don't expect rigid application of precise formatting guidelines; just avoid anything too inconsistent with default Eclipse layout.

Review results
Some review comments can probably be resolved very quickly and so just done.
Anything affecting API needs to be done as soon as possible.
Anything affecting accuracy should be done soon, but some things may be hard and so left in a Bugzilla.
It would be good to tackle performance soon as well, but what cannot be done should go to Bugzilla.

Please branch the code on GitHub and commit your refactorings and distinctively tagged comments such as // REVIEW or // BUG. Please avoid mixing name refactorings and review comments in the same commit, since refactorings tend to produce large deltas; individual comments will get lost. (One day we might have Gerrit).


        Ed Willink

On 03/02/2012 10:23, Michael Golubev wrote:

Below is update from the GMF-T:

For last 3 weeks we were working on the integration of the ImpactAnalyzer into the GMFT-generated diagrams, 
as part of the Bugzilla #368398 -- modeling support for diagram properties auto-updated based on semantic properties of the element. 

As part of this work we have implemented 50+ tests for IA (right now they depends on the GMF-T), and manually tested the generated diagrams (that uses IA) with fairly complex OCL expressions. We were focusing on the use cases affecting less than a 100 semantic elements, as it is the primary use case for diagramming. 

So far we have't found any problems and we would like to promote the work done for #368398 and # into the GMF-T master. 

Even more, in case if IA would be promoted from an examples to be a part of the MDT OCL, we will consider rewriting the base diagram updater mechanism for using with IA, because it is now clear that it would lead to a simpler and more stable diagram code than the one we are actually generating for diagram update. 

Thus, I am definitely +1 for promoting the IA to the main OCL distribution. 


Michael "Borlander" Golubev
Eclipse Committer (GMF, UML2Tools)
at Montages Think Tank, 
Prague, Czech Republic

Montages AG
Stampfenbachstr. 48, CH-8006 Zürich

tel:    +41 44 260 75 57
mob: +420 602 483 463

On Thu, Feb 2, 2012 at 11:55 AM, Axel Uhl <eclipse@xxxxxxxxxxxxxxxxxx> wrote:

I can only say that I fully agree with your assessment. We know what to do: get the IA review done and work on promoting it from examples/ to plugins/ for the "mature" ecore implementation.

-- Axel

On 1/4/2012 11:51 AM, Philipp W. Kutter | Montages AG wrote:
GMF-T offered to help with the review of the IA and to promote it to the
MDT OCL main component.

AFAIK the IA only works for the Stable/Old implementation, which is as
well used in GMF-T currently. Thus while it will of course help in the
future to extend the IA to the Pivot/New, it would currently improve the
Stable/Old implementation.

Has the offer been accepted by the MDT OCL project? I have not seen a
clear answer in this mailing list. I have seen prototypes of simply
using the example component (thus code duplication!), and an own GMF-T
IA implemenation, both of them working.

I assume, we all agree that using the IA from the MDT OCL project in
GMF-T would be the ideal solution.

A clear answer to the offer of the GMF-T would be very good!

Regards, Philipp

_______________________________________________ mailing list

_______________________________________________ mailing list

_______________________________________________ mailing list

No virus found in this message.
Checked by AVG -
Version: 2012.0.1913 / Virus Database: 2112/4782 - Release Date: 02/02/12

Back to the top