Bug 349117

Comment 1 Axel Uhl 2011-06-12 19:45:59 EDT

Created attachment 197866 [details]
Failing test case

Shows in TupleTest how we currently fail for a Long value being put into a tuple. See also GIT branch bugs/349117.

Comment 2 Axel Uhl 2011-06-12 19:51:06 EDT

Created attachment 197867 [details]
Showing it fails even without using ELong

The test even fails if no ELong is being used, simply by performing a multiplication that coerces the int to a long internally, hence becoming unassignable.

Comment 3 Axel Uhl 2011-06-28 10:40:53 EDT

Created attachment 198738 [details]
Patch that uses java.lang.Long as Integer instance type

Had to adjust a few test cases that assumed the result will be an int or java.lang.Integer. Committed on branch bug/349117.

Comment 4 Axel Uhl 2011-06-29 08:43:54 EDT

Ed, to appease your concerns regarding the proposed change of the Integer implementation type from java.lang.Integer to java.lang.Long, there is no automatic conversion taking place that would take a java.lang.Integer object and convert it to a java.lang.Long implicitly, except for the one case where a java.lang.Integer needs to be assigned to a tuple component that has Java implementation type java.lang.Long.

Given that the OCL evaluator handles Integer values as the java.lang.* object wrappers around values of primitive type, we're talking about a 32/64 bit (4 bytes) reference to a 32 bit (4 bytes) int or 64 bit (8 bytes) long plus the object header which at least is 8 bytes on 32-bit platforms.

http://stackoverflow.com/questions/258120/what-is-the-memory-consumption-of-an-object-in-java explains that a java.lang.Integer wrapper object due to memory layout considerations (aligning along 8 byte boundaries performs better than aligning along 12 byte boundaries) consumes exactly as much space as a java.lang.Long object.

A further case in point is the success of 64bit processors and the corresponding operating systems which make the difference even less important.

If you take a close look at my patch you see that the only test case verdict changes were in the areas where tuples containing OCL Integer components are tested and compared to a Java object.

The migration hint to users is obviously very straightforward: if anyone extracts an Integer from a tuple, the Java object type of the result will be Long. Comparisons with auto-boxed int values will have to be adjusted such that a long value is auto-boxed. I find this acceptable given that it would allow us to fix a show stopper that currently makes it impossible to use long values in tuples.

Comment 5 Ed Willink 2011-06-29 18:12:52 EDT

I haven't looked at the code yet but on reflection I cannot see why there should be a problem.

I'm guessing that there is an incompatibility because you have neglected to invoke coerceValue to minimize the return value. Whether the change in internal data type is an issue may take careful study. It may be that the tuple internals are 100% encapsulated and so you can change format arbitrarily, but since the mature evaluator returns naked values the tuple innards may be accessible, so compatibility needs assessing. Even if the internal value is externally visible I see no reason why the tuple content shouldn't be Integer for int-sized values preserving existing compatibility and Long for enhanced functionality that was previously broken.

64/32/99 bit processors is a red herring. The code either is or is not compatible for our current users for whom we only support int behaviour reliably. A valid existing program must not be broken by this change. Changing a test verdict is a strong hint that the change is externally visible on a valid program.

If this change is to make Long values a supported property then Long must be addressed throughout the evaluator. See bug 344368. I see no point in improving support for Long from 10% of possible usage to 20%. >90%, preferably 100% yes.

Comment 6 Axel Uhl 2011-06-30 02:58:15 EDT

(In reply to comment #5)
> I haven't looked at the code yet but on reflection I cannot see why there
> should be a problem.

Good :-)

> I'm guessing that there is an incompatibility because you have neglected to
> invoke coerceValue to minimize the return value.

Calling coerceValue before assigning to the tuple component would cause an exception. java.lang.Integer does not conform to java.lang.Long. The tuple type has to be created with java.lang.Long as the component's type to permit assignment of long values. Rather than coercing a long to java.lang.Integer it is necessary to convert an int/java.lang.Integer to a java.lang.Long which the patch does.

I've now used NumberUtil.coerceNumber to ensure that when a PropertyCallExp returns a number then it is coerced, as well as if a getValue is called on a Tuple object with a Number result it is coerced. With this I was able to revert all test verdict changes.

Your mentioned in our discussion that one should try to infer whether the Long range is required by the tuple. I think this is hardly possible as long as the tuple types are associated with, cached by and created based on their OCL component types. The EMF component type for the tuple component is selected based on the type information only and without knowledge about any possible values. I expect major problems in case we decided to have different component types for OCL Integer-typed tuple components based on additional "magic" knowledge about the component's value range. Besides, inferring the value range would require evaluating, e.g., a tuple literal expression, but the type for the tuple component already needs to be determined at compile time. Therefore, a fixed mapping of OCL Integer to java.lang.Long during tuple type construction seems the only way out.

> 64/32/99 bit processors is a red herring. The code either is or is not

I just mentioned it because in our discussion you brought it up while you were arguing about memory consumption. Good we agree that there is no significant memory footprint issue based on our choice in this case.

> compatible for our current users for whom we only support int behaviour
> reliably. A valid existing program must not be broken by this change. Changing

As usual we assess "break" differently. The additional coerceNumber can help avoid a change in test verdicts as we've seen.

Comment 7 Axel Uhl 2011-06-30 03:00:10 EDT

Created attachment 198874 [details]
Using coerceValue to go with Integer where possible

See previous discussion and bug branch in git

Comment 8 Ed Willink 2011-07-13 05:11:38 EDT

See comments on Bug 344368.

Where are the tests for the isKindOf and isTypeOf? what is the classifier == OCLStandardLibraryImpl.INSTANCE.getInteger() term for? It is unlike anything else.

Comment 9 Axel Uhl 2011-07-13 14:56:40 EDT

> See comments on Bug 344368.

A comprehensive list. Any way we can approach this incrementally?

> Where are the tests for the isKindOf and isTypeOf? what is the classifier ==

There are several tests, e.g., in EvaluationNumberOperationTest.[testNumberOclIsKindOf|testNumberOclIsTypeOf|testUnlimitedOclIsKindOf|testUnlimitedOclIsTypeOf].

> OCLStandardLibraryImpl.INSTANCE.getInteger() term for? It is unlike anything
> else.

That's how I tried to get a hold of what the stdlib thinks is the Integer type. Is there any better, more compact way to get a hold of the EClassifier representing the OCL stdlib's Integer type?

Comment 10 Ed Willink 2011-07-13 15:21:44 EDT

(In reply to comment #9)
> A comprehensive list. Any way we can approach this incrementally?

Probably, depending on what you want to achieve for SR1/Juno. Nothing can be decided until the API consequences of the Library instanceClass are understood/worked around. Everything else seems relatively easy.

> 
> > Where are the tests for the isKindOf and isTypeOf? what is the classifier ==
> 
> There are several tests, e.g., in
> EvaluationNumberOperationTest.[testNumberOclIsKindOf|testNumberOclIsTypeOf|testUnlimitedOclIsKindOf|testUnlimitedOclIsTypeOf].

Yes, but no new tests for Long.

Comment 11 Axel Uhl 2011-07-13 17:45:11 EDT

(In reply to comment #10)
> > There are several tests, e.g., in
> > EvaluationNumberOperationTest.[testNumberOclIsKindOf|testNumberOclIsTypeOf|testUnlimitedOclIsKindOf|testUnlimitedOclIsTypeOf].
> 
> Yes, but no new tests for Long.

Ok, added as EvaluationNumberOperationTest.[testLongOclIsKindOfInteger|testLongOclIsKindOfReal|testLongOclIsTypeOf] on branch bug/344368 which now has bug/349117 merged in and provides the common patch.

Comment 12 Ed Willink 2011-07-24 12:43:21 EDT

Although I've yet to see the explanation of why Integer.instanceClass needs to be changed, I've done a preliminary review anyway.
Not a pleasant undertaking with EGIT, whose Compare functionality is barely useable.

Integer.instanceClass
---------------------

I see no need for this change. The only test that 'breaks' when restored is a new test, which seems to indicate that
undue reliance is placed on the instanceClass. If the OCL type is integer, then an OCL integer representation should
be provided. This is now a coercedNumber, i.e. java.lang.Integer whenever possible to preserve API compatibility and
java.lang.Long to support new valid functionality where previously broken.

Therefore EcoreEvaluationEnvironment.createTuple must react to the OCL type and can ignore the Java type so there
is no need to change the Java type, except that it might be interesting to change it to java.util.Date to verify
that the value is never actually used.

API Breakage
------------

The API tooling reports a need for a major version number change because presumably something, unhelpfully not identified,
is not compatible. There are plenty of unnecessary changes identified below so with luck this might go away so that this
can be committed for Juno or an early 3.2. Sadly no chance of a 3.1.1 approval.

Memory size
-----------

The change to support Long values is to support greater than 32 bit numbers. It is not possible to support greater
than 31 bit collection sizes or ordered collection indexes since:
- java.lang.Collection.size() returns int
- CollectionUtil.indexOf returns Integer
- CollectionUtil.count returns Integer
- PredefinedType.SIZE doesn't handle longs
[unless you care to re-implement Collection in 31-bit address pages.]

All the changes to use long rather than int arguments throughout CollectionUtil and IntegerRangeList are therefore
mostly invalid and only likely to give useful results in fortuitous circumstances. These problems would have
been revealed if some tests had been provided for this significantly changed functionality.

Retract all changes for greater than 31 bit indexes, and since there is a significant 64 bit/31 bit boundary
introduce a solid invalid with an 'ArrayIndexOutOfBoundsException' in the log.

[Incidentally, the Indigo pivot has int for indexes; it is only with introduction of
IntegerIntValue that Long and BigInteger indices happen to be easier to support than not.
I'll need to review whether the pivot has an adequate detection of Java address limitations.]

LongLiteralExp
--------------

I'm very uncomfortable with LongLiteralExp extending IntegerLiteralExp so that a LongLiteralExp has two
unsynchronized values and an additional getSymbol method with no clear purpose. I'm not at all clear whether
an existing derived visitor will work well. Make IntegerLiteralExp.integerSymbol a java.langNumber and manually
code the set/get to preserve the existing Integer API on setIntegerSymbol, provide new API on setLongSymbol,
and only issue a single INTEGER_LITERAL_EXP__INTEGER_SYMBOL notification with coerced value whichever
form of set is actually used. For compatibility I presume that getIntegerSymbol returns .intValue() no
matter how stupid that may be.

IntegerLiteralExpCSImpl
-----------------------

Same recommendation as LongLiteralExp; single API compatible java.lang.Number field.

This makes the 'changes' to AbstractOCLParser, AbstractOCLAnalyzer almost trivial.

Javadoc
-------

Comments needed on at least IntegerLiteralExp, IntegerLiteralExpCS, NumberUtil.coerceNumber, CollectionUtil.sum

Evaluation
--------

The following fail

		assertResult(2147483648L, "-(-1-2147483647)");

		assertResult(2222222222L, "2222222222.toInteger()");

		assertResult(2147483648L, "Sequence{2147483647..2147483648, -2147483647}->sum()");

		assertResult(2147483648.0, "2147483648.oclAsType(Real)");

PredefinedType.OCL_AS_TYPE: Long not needed for unlimitedNatural; the new untested code would CCE if executed

TupleFactory: why doesn't getValue by part coerce?

visitTupleLiteralExp: why no coerceNumber for tuple elements

Generally, there should be a policy to coerce-on-create or coerce-on-use. coerce-on-create is more efficient since there
are more uses than creates.

AbstractEvaluationVisitor.visitExpression: coerceNumber should be unnecessary, since it is neither, just a hopeful
make the bugs go away by doing a coerce-on-pass. navigateProperty is one clear example of a bad coerce-on-create
though I wouldn't really want to touch it because it invokes coerceValue which is very magical sorting out
XSD -2 multiplicities in a seemingly telepathic fashion.

visitAssociationClassCallExp is also missing coerceNumber

visitCollectionLiteralExp has some very strange treatment of null. Does this work for Longs; I would like to see tests
of hybrid Long/Integer collections.

Tests
-----

One of the good things about Laurent's tests was the localization of functionality, which I've taken further in the pivot variant by merging Null/Invalid variants.

Introducing new tests miles away from their counterparts is unhelpful. Either put them adjacent or embed as sub-tests.

The change for Long as well as Integer is big and should be correspondingly tested, I would expect to see sub-tests such as the pivot tests

		assertQueryTrue(null, "2147483648 > 2147483647");
		assertQueryFalse(null, "2147483647 > 2147483648");
		assertQueryFalse(null, "-2147483649 > -2147483648");
		assertQueryTrue(null, "-2147483648 > -2147483649");

		assertQueryTrue(null, "9223372036854775807 > 9223372036854775806");
		assertQueryFalse(null, "9223372036854775806 > 9223372036854775807");
		assertQueryFalse(null, "(-9223372036854775807-1) > -9223372036854775807");
		assertQueryTrue(null, "-9223372036854775807 > (-9223372036854775807-1)");

in all relevant numeric tests.

The change for testNumberOclAsType() from assertResult(Double.valueOf(3), "3.oclAsType(Real)") to assertResult(Double.valueOf(3), "3.oclAsType(Real)") is unnecessary.

There are no tests for negate. The pivot tests are:
		
	public void testNumberNegate() {
		assertQueryEquals(null, -1, "-1");
		assertQueryEquals(null, -1.0, "-1.0", 0.0);

		assertQueryEquals(null, -2147483647, "-2147483647");
		assertQueryEquals(null, -2147483648, "-2147483648");
		assertQueryEquals(null, -2147483649L, "-2147483649");

		assertQueryEquals(null, BigInteger.ONE.shiftLeft(63).negate().add(BigInteger.ONE), "-9223372036854775807");
		assertQueryEquals(null, BigInteger.ONE.shiftLeft(63).negate(), "-9223372036854775808");
		assertQueryEquals(null, BigInteger.ONE.shiftLeft(63).negate().subtract(BigInteger.ONE), "-9223372036854775809");
		// invalid
		assertQueryInvalid(null, "let i : Integer = invalid in -i");
		assertQueryInvalid(null, "let r : Real = invalid in -r");
		// null
		assertQueryInvalid(null, "let i : Integer = null in -i");
		assertQueryInvalid(null, "let r : Real = null in -r");
	}

There are no tests on Integer/Long property access.

Code formatting
---------------

There are numerous trivial differences; a blank line after/among imports, enumeration of import statics in OCLStandardLibraryUtil.
Are you now using a stable code formatting, so that we might do better in the future, or is this just a difference between two
accidental styles? If so, you might care to do https://bugs.eclipse.org/bugs/show_bug.cgi?id=352945 to pick up the formatting
changes and Switch inheritance as a preliminary maintenance so that this bug only has real changes.

Comment 13 Axel Uhl 2011-07-25 05:01:05 EDT

(In reply to comment #12)

A quick first comment to your first point:

> Integer.instanceClass
> ---------------------
> 
> I see no need for this change. The only test that 'breaks' when restored is a
> new test, which seems to indicate that
> undue reliance is placed on the instanceClass. If the OCL type is integer, then
> an OCL integer representation should
> be provided. This is now a coercedNumber, i.e. java.lang.Integer whenever
> possible to preserve API compatibility and
> java.lang.Long to support new valid functionality where previously broken.
> 
> Therefore EcoreEvaluationEnvironment.createTuple must react to the OCL type and
> can ignore the Java type so there
> is no need to change the Java type, except that it might be interesting to
> change it to java.util.Date to verify
> that the value is never actually used.

The property type used for a tuple part with type Integer is the OCL stdlib Integer classifier. The tuple instance is a DynamicEObject of type TupleInstance. When an eSet is invoked on it for the Integer property, EMF checks the Object representing the value to be set for conformance with the property's type. It is this conformance check that fails. We'd have to redefine eSet and avoid type checks against the instanceClass for at least TupleInstance. We'd end up with an inconsistency from an EMF point of view because we would assign a non-conforming value to a property. I wouldn't dare to do so, given that I don't know where a TupleInstance may be used by other EMF clients who rely on a property's value to conform to the type's instanceClass if provided.

Are you suggesting we should accept this inconsistency and simply force a java.lang.Long value into a property of which EMF things it conforms to java.lang.Integer?

Comment 14 Ed Willink 2011-07-25 05:13:48 EDT

We certainly must not abuse EMF. Even if it works today, it will be fragile.

As far as I can see the instanceClass declarations in the OCL stdlib 'models' are not directly used by EMF, since the contexts are not EDataTypes. It should be possible to ensure that EMF is using a java.lang.Number for its validation.

Comment 15 Axel Uhl 2011-07-25 06:49:59 EDT

(In reply to comment #14)
> We certainly must not abuse EMF. Even if it works today, it will be fragile.
> 
> As far as I can see the instanceClass declarations in the OCL stdlib 'models'
> are not directly used by EMF, since the contexts are not EDataTypes. It should
> be possible to ensure that EMF is using a java.lang.Number for its validation.

Below is the stack trace where things go wrong. You can see that the eSet on the tuple instance leads to a ClassCastException in the standard EMF EStructuralFeatureImpl code. Suggestions?

Thread [main] (Suspended (exception ClassCastException))	
	EStructuralFeatureImpl$InternalSettingDelegateSingleDataStatic.validate(Object) line: 2131	
	EStructuralFeatureImpl$InternalSettingDelegateSingleDataStatic(EStructuralFeatureImpl$InternalSettingDelegateSingleData).dynamicSet(InternalEObject, EStructuralFeature$Internal$DynamicValueHolder, int, Object) line: 2056	
	TupleFactory$TupleInstance(BasicEObjectImpl).eDynamicSet(int, EStructuralFeature, Object) line: 1137	
	TupleFactory$TupleInstance(BasicEObjectImpl).eSet(int, Object) line: 1111	
	TupleFactory$TupleInstance(BasicEObjectImpl).eSet(EStructuralFeature, Object) line: 1081	
	EcoreEvaluationEnvironment.createTuple(EClassifier, Map<EStructuralFeature,Object>) line: 419	
	EcoreEvaluationEnvironment.createTuple(Object, Map) line: 1	
	EvaluationVisitorImpl(EvaluationVisitorImpl<PK,C,O,P,EL,PM,S,COA,SSA,CT,CLS,E>).visitTupleLiteralExp(TupleLiteralExp<C,P>) line: 2488	
	TupleLiteralExpImpl.accept(U) line: 242	
	EvaluationVisitorImpl(EvaluationVisitorImpl<PK,C,O,P,EL,PM,S,COA,SSA,CT,CLS,E>).visitPropertyCallExp(PropertyCallExp<C,P>) line: 2061	
	PropertyCallExpImpl.accept(U) line: 238	
	EvaluationVisitorImpl(AbstractEvaluationVisitor<PK,C,O,P,EL,PM,S,COA,SSA,CT,CLS,E>).visitExpression(OCLExpression<C>) line: 248	
	OCL(OCL<PK,C,O,P,EL,PM,S,COA,SSA,CT,CLS,E>).evaluate(Object, OCLExpression<C>) line: 456	
	TuplesTest(GenericTestSuite<E,PK,T,C,CLS,DT,PT,ET,O,PM,P,PA,PR,EL,S,COA,SSA,CT>).evaluate(OCLExpression<C>, Object) line: 721	
	TuplesTest.test_tupleWithLongIntegerProperty_349117() line: 267	
	NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]	
	NativeMethodAccessorImpl.invoke(Object, Object[]) line: 39	
	DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 25	
	Method.invoke(Object, Object...) line: 592	
	TuplesTest(TestCase).runTest() line: 168	
	TuplesTest(TestCase).runBare() line: 134	
	TestResult$1.protect() line: 110	
	TestResult.runProtected(Test, Protectable) line: 128	
	TestResult.run(TestCase) line: 113	
	TuplesTest(TestCase).run(TestResult) line: 124	
	TestSuite.runTest(Test, TestResult) line: 243	
	TestSuite.run(TestResult) line: 238	
	JUnit38ClassRunner.run(RunNotifier) line: 83	
	JUnit4TestMethodReference(JUnit4TestReference).run(TestExecution) line: 50	
	TestExecution.run(ITestReference[]) line: 38	
	RemoteTestRunner.runTests(String[], String, TestExecution) line: 467	
	RemoteTestRunner.runTests(TestExecution) line: 683	
	RemoteTestRunner.run() line: 390	
	RemoteTestRunner.main(String[]) line: 197

Comment 16 Axel Uhl 2011-07-25 07:48:25 EDT

Ed, you wrote:
> The change for testNumberOclAsType() from assertResult(Double.valueOf(3),
> "3.oclAsType(Real)") to assertResult(Double.valueOf(3), "3.oclAsType(Real)")
> is unnecessary.

I can't see a change, neither in the diff nor in your statement. Typo? Am I missing something?

Comment 17 Ed Willink 2011-07-25 12:17:30 EDT

(In reply to comment #16)
> Ed, you wrote:
> > The change for testNumberOclAsType() from assertResult(Double.valueOf(3),
> > "3.oclAsType(Real)") to assertResult(Double.valueOf(3), "3.oclAsType(Real)")
> > is unnecessary.
> 
> I can't see a change, neither in the diff nor in your statement. Typo? Am I
> missing something?

Well, this only adds to my EGIT discomfort. Today, I see something different, but still see a difference between the branch and origin/master. What I saw yesterday was much closer to a difference between three commits ago and master.

My comment indeed has a cut and paste typo, the test expectation has changed unnecessarily from Double.valueOf(3) to 3.0 or new Double(3).

Comment 18 Ed Willink 2011-07-25 12:50:12 EDT

(In reply to comment #15)
> Below is the stack trace where things go wrong. You can see that the eSet on
> the tuple instance leads to a ClassCastException in the standard EMF
> EStructuralFeatureImpl code. Suggestions?

It looks like you have found another problem in the very suspect design of Tuples. You should look at bug 350493 which identifies the current design as invalid.

I presume that the design rationale was that a Tuple with properties is similar to an EClass with EStructuralFeatures and so the reuse of EClass and EFactory could save code. Having directly implemented a TupleValue for the pivot model, I can see no merit in the re-use and considerable inconvenience. Bug 350493 undermines the design. The need to comply with EStructuralFeature validation is causing your CCE, though I suspect that you could arrange for the dynamically constructed EStructuralFeature to be for a Number rather than an Integer.

The current design is a mess. The challenge is to come up with an improvement that is API compatible, existing users accessing 32-bit values must get an Integer, enhanced users may get a Long.

Tuple is a public interface and is reasonable. The UML derivation is private and sensible. The Ecore derivation in TupleFactory is in an internal package. I recommend reimplementing Ecore Tuples, possibly borrowing from the UML implementation, possibly borrowing from the pivot implementation and eliminating the Value polymorphism.

Comment 19 Axel Uhl 2011-07-25 14:54:18 EDT

> LongLiteralExp
> --------------
> 
> I'm very uncomfortable with LongLiteralExp extending IntegerLiteralExp so that
> a LongLiteralExp has two
> unsynchronized values and an additional getSymbol method with no clear purpose.
> I'm not at all clear whether
> an existing derived visitor will work well. Make
> IntegerLiteralExp.integerSymbol a java.langNumber and manually
> code the set/get to preserve the existing Integer API on setIntegerSymbol,
> provide new API on setLongSymbol,
> and only issue a single INTEGER_LITERAL_EXP__INTEGER_SYMBOL notification with
> coerced value whichever
> form of set is actually used. For compatibility I presume that getIntegerSymbol
> returns .intValue() no
> matter how stupid that may be.

We'd first have to introduce an EDataType whose implementation type is java.lang.Number. I don't think this exists yet. This would then need to be used for IntegerLiteralExp.integerSymbol as well as the ...CS class where currently ecore.EIntegerObject is being used.

However, I'm afraid this would still not be API-compatible, particularly because getIntegerSymbol() would now be more general, making assignments to java.lang.Integer or int fail. Before we move this forward, let me know if you think this sort of change would be ok.

Comment 20 Ed Willink 2011-07-25 16:03:03 EDT

(In reply to comment #19)
> We'd first have to introduce an EDataType whose implementation type is
> java.lang.Number. I don't think this exists yet. This would then need to be
> used for IntegerLiteralExp.integerSymbol as well as the ...CS class where
> currently ecore.EIntegerObject is being used.
A bit inelegant, not a big problem, but...
> 
> However, I'm afraid this would still not be API-compatible, particularly
> because getIntegerSymbol() would now be more general, making assignments to
> java.lang.Integer or int fail. Before we move this forward, let me know if you
> think this sort of change would be ok.

No. API compatibility requires an Integer return. You must use the correct permutation of @generated NOT, transient, volatile, derived and manual code, which may also avoid the need for an EDataType.

Comment 21 Axel Uhl 2011-07-25 17:10:36 EDT

(In reply to comment #20)
> > However, I'm afraid this would still not be API-compatible, particularly
> > because getIntegerSymbol() would now be more general, making assignments to
> > java.lang.Integer or int fail. Before we move this forward, let me know if you
> > think this sort of change would be ok.
> 
> No. API compatibility requires an Integer return. You must use the correct
> permutation of @generated NOT, transient, volatile, derived and manual code,
> which may also avoid the need for an EDataType.

Sorry, too cryptic for me. If getIntegerSymbol() does not, contrary what I read from you earlier, return a java.lang.Number but a java.lang.Integer, then how does it deal with Long values?

Comment 22 Ed Willink 2011-07-26 01:35:54 EDT

(In reply to comment #20)
> No. API compatibility requires an Integer return. You must use the correct
> permutation of @generated NOT, transient, volatile, derived and manual code,
> which may also avoid the need for an EDataType.

This is precisely the mightmare that made me think it was just too hard to upgrade the mature code.

transient, volatile are not a primary part of the solution because the AST must be persistable (for e.g. Acceleo) and copyable.

(In reply to comment #21)
> Sorry, too cryptic for me. If getIntegerSymbol() does not, contrary what I read
> from you earlier, return a java.lang.Number but a java.lang.Integer, then how
> does it deal with Long values?

----------------------
API compatibility for an extended one class approach requires:

getIntegerSymbol()/setIntegerSymbol() use Integer always
integerSymbol is serializable as a string always
eGet/eSet use Integer when possible
xx__INTEGER_SYMBOL notifies changes using an Integer when possible

integerSymbol can be ELongObject or 'ENumberObject' with getIntegerSymbol()/setIntegerSymbol() @generated NOT to get compatibility, and getLongSymbol()/setLongSymbol() added manually.

But eGet invokes getIntegerSymbol by default, so eGet/eSet must also be @generated NOT to get extensibility. Inelegant but not much code.

convertXXXToString and convertStringToXxx sort out serializability for a larger integerSymbol. My earlier suggestion of an lsw/msw approach won't work because the serializer won't be aware of the msw counterpart of the lsw.

Changes to usage code are easy, just use get/setLong.

-----------------------
API compatibility for an 'independent' second class requires:

mmm... nothing, just binary compatibility. The extra visitLongLiteralExp case in switches/visitors must not disrupt applications that don't implement. For the most part the default null implementation in the abstract Switch/Visitor will do that. 

Changes to calling code are a bit harder. Unfortunately the new class name leaks out into the persisted AST. However provided new only occurs for users using new functionality so no problem and no horrible tricky @generated NOT as required for one class.

------------------
If LongLiteral extends IntegerLiteral the is-a is counter-intuitive, and there is a persistence stupidity whereby both 32 and 64 bit fields are serialized, and 32 bit switch/visitor cases will be offered 64 bit values.

If IntegerLiteral extends LongLiteral the is-a is sensible, but now 32 bit values persist a 64 bit value too, breaking the no use no change requirement.

So I suppose LongLiteral must be 100% independent of IntegerLiteral, just another NumericLiteral.

-------------------
Conclusion: an extended IntegerLiteral class is too tricky, perhaps impossible.

An additional independent LongLiteral class can be internally simple and API safe provided a LongLiteral is only created for >32 bit values. Unfortunately all IntegerLiteral handling code must now be adjusted for two unrelated integer classes.

As noted before, since applications may not all be recoded to support LongLiteral, there must a facility in for instance Window->Preferences->OCL to suppress creation of LongLiteral instances.

Comment 23 Ed Willink 2012-05-01 08:53:52 EDT

bug/344368b has a plausible resolution of the enhancement for Integer/Long using an lsw and optional extended precision msw so that serialization is ok.

Using coerceNumber resolves problems for Tuple part read.

Tuple part write is a significant problem because the part type identifying the OCL Integer type is fine OCL-wise, but inadequate Ecore-wise when used for both Long and Integer.

The Ecore Tuple Factory/Instance support is very 'internal' so fixes are possible. The Tuple part type cannot change but the need for it to be an Ecore type descriptor is not necessary. I think compatibility just requires use of the Tuple<O, P> interface.

The Ecore Tuple support adds complexity to the UML tuple support. Perhaps we can just throw away the Ecore support that could have been simplified when UML/Ecore duality was introduced. The pivot support is similarly simplified.

Comment 24 Ed Willink 2012-05-01 16:23:22 EDT

bug/344368b has a solution with the UML TupleImpl shared by Ecore; the Ecore approach with an EFactory seesm to have been there solely to cause needless problems and inefficiencies through use of the full eGet()/eSet() call chains.

The TupleTests are now shared across UML and Ecore too and now work the same on both. The legacy implementation has the odd feature that a Set of tuples may have a different type for each set element, whereas the pivot establishes a common type.

Comment 25 Ed Willink 2012-05-04 13:36:35 EDT

Pushed to master.

Comment 26 Ed Willink 2013-05-20 11:37:52 EDT

CLOSED after a year in the RESOLVED state.