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?
Algorithms
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?
Caches
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
org.eclipse.ocl.ecore.impact.runtime
org.eclipse.ocl.ecore.impact.analyzer
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
org.eclipse.ocl.domain.impact.runtime
Migration of the meta-run-time code will be harder because that
obviously makes use of the OCL meta-model. Perhaps
org.eclipse.ocl.pivot.impact.analyzer
In order to avoid code duplication, code that is independent of
Ecore/UML/Pivot should be in perhaps
org.eclipse.ocl.common.impact
It may also be appropriate to place some declarations independent of
Ecore/UML/Pivot such as extension points in
org.eclipse.ocl.common
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.
Naming
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.
Tests
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?
Layout
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).
Regards
Ed Willink
On 03/02/2012 10:23, Michael Golubev wrote:
Hello,
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.
Regards,
--
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:
Philipp,
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.
Best,
-- 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
_______________________________________________
mdt-ocl.dev mailing list
mdt-ocl.dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mdt-ocl.dev
_______________________________________________
mdt-ocl.dev mailing list
mdt-ocl.dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mdt-ocl.dev
_______________________________________________
mdt-ocl.dev mailing list
mdt-ocl.dev@xxxxxxxxxxx
https://dev.eclipse.org/mailman/listinfo/mdt-ocl.dev
No virus
found in this message.
Checked by AVG - www.avg.com
Version: 2012.0.1913 / Virus Database: 2112/4782 - Release Date:
02/02/12
|