search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[evaluator] Tuple construction with long-typed components fails #720

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 349117 | | Status | CLOSED FIXED | | Importance | P3 major | | Reported | Jun 11, 2011 07:22 EDT | | Modified | May 20, 2013 11:37 EDT | | Version | 3.1.0 | | Depends on | 344368 | | Blocks | 350493 | | Reporter | Axel Uhl |

Description

During construction of a Tuple there is some confusion regarding the type conversion of java.lang.Integer / java.lang.Long into an OCL Integer. The OCL Integer type has as its implementation type java.lang.Integer. However, the evaluator can also work with java.lang.Long. This is no problem unless the OCL evaluator needs to construct EClassifiers on the fly. Then, when it finds a java.lang.Long it infers OCL Integer as the type, constructs a corresponding TupleType based on the OCL Integer representation type which is java.lang.Integer and then tries to assign which fails.

I'll construct a test case reproducing the scenario.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 12, 2011 19:45

Created attachment 197866 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 12, 2011 19:51

Created attachment 197867 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.

:notepad_spiral: 349117.patch

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 28, 2011 10:40

Created attachment 198738 (attachment deleted)\ 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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 29, 2011 08:43

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jun 29, 2011 18:12

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 30, 2011 02:58

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jun 30, 2011 03:00

Created attachment 198874 Using coerceValue to go with Integer where possible

See previous discussion and bug branch in git

:notepad_spiral: 349117.patch

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 13, 2011 05:11

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 13, 2011 14:56

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?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 13, 2011 15:21

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 13, 2011 17:45

(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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 24, 2011 12:43

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:

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 25, 2011 05:01

(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?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 25, 2011 05:13

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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 25, 2011 06:49

(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) line: 248 \ OCL(OCL<PK,C,O,P,EL,PM,S,COA,SSA,CT,CLS,E>).evaluate(Object, OCLExpression) line: 456 \ TuplesTest(GenericTestSuite<E,PK,T,C,CLS,DT,PT,ET,O,PM,P,PA,PR,EL,S,COA,SSA,CT>).evaluate(OCLExpression, 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

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 25, 2011 07:48

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?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 25, 2011 12:17

(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).

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 25, 2011 12:50

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 25, 2011 14:54

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 25, 2011 16:03

(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.

eclipse-ocl-bot commented 1 month ago

By Axel Uhl on Jul 25, 2011 17:10

(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?

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 26, 2011 01:35

(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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 01, 2012 08:53

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 01, 2012 16:23

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.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 04, 2012 13:36

Pushed to master.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 20, 2013 11:37

CLOSED after a year in the RESOLVED state.