Community
Participate
Working Groups
Continuing from Bug 342644 (In reply to comment #66) > What exactly is the problem with Long/Integer? The Integer/Long/Double issue for evaluation is much simpler than I thought, but still unpleasant. Evaluation is fully supported for Java's Integer and Double. Evaluation is mostly supported for Java's Long; the NFE comment in NumberUtil.coerceNumber is wrong. ObjectUtil.isPrimitive enumerates the supported types. Evaluation support for BigDecimal and BigInteger is limited to ObjectUtil.equal. If lucky, gratuitously big numbers can be coerced correctly to Integer/Double, but the handling of BigInteger to Long is missing and the requisite conversion is not certain to occur without a CCE. Integer/Double overflows cause silent arithmetic malfunction. Integer does not promote to Long. A sequence of calculations involving Long may CCE. - bug: CollectionUtil.sum CCEs for Long. Numbers must be coerced to Integer/Long/Double by the source, since numerous Integer-only recipients do not convert. - bug: PropertyCallExp does not coerce incoming numerics UML/EcoreEvaluationEnvironment.isTypeOf/isKindOf do not handle Long. UML/EcoreEvaluationEnvironment.isTypeOf uses getInstanceClass so changing UnlimitedNatural to have a java.lang.Integer will change the behaviour of Integer/UnlimitedNatural tests which previously reported different types correctly but for the wrong reason, to report equal types incorrectly. Looking again at the 13 usages of getInstanceClass(). I think that just about every one has an associated bug either for Java Integer/Long or OCL Integer/UnlimitedNatural. The usage of instanceTypeName for built-in types is a dangerous complexity. I recommend eliminating all usages of getInstanceClass for OCL types. Retain only those usages that extend from OCL to Ecore and user data types. The diverse null-ness of oclstdlib.ecore instanceTypeName is then irrelevant.
Long is not currently supported, although the lack of support is only stated in an obscure comment. The lack of support is a clear violation of the specification and so a fix is well within scope for a service release, provided it is API compatible and only changes results when a non-32 bit value is in use. Long is not supported in the parser, because there is a programmed number range trap and because IntegerLiteralExpCSImpl.integerSymbol is an Integer. Not even the full 32 bit range is supported for UnlimitedNatural. Long is not supported in the AST, IntegerLiteralExp.integerSymbol is an Integer. Long is not supported in the Library, in OCLStandardLibraryImpl there is "OCL_INTEGER.setInstanceClass(Integer.class)". Long is substantially supported but almost totally untested in the evaluator. oclKindOf and oclTypeOf lack support, and numeric growth fails: e.g. assertResult(2147483648L, "(-1-2147483647).abs()"); and a Fibonnaci series evaluation generates negative numbers after overflow. Long is not consistently supported for property and tuple access; see Bug 349117. -------- Parser and AST inadequacies can perhaps be fixed by introducing LongLiteralExp*. Any API issue is hopefully limited to the 'permissible' evolution of EMF Factory/Package. The evaluator can be tested and debugged for Long and the Long/Integer boundary. Should not present an API issue. Field accesses can be fixed as in Bug 349117, and uncoerceable BigInteger and BigDecimal should be trapped at source rather than allowed to fail at some indeterminate point. BigInteger and BigDecimal are not supported by the Ecore evaluator. Changing the library to have Long.class as the instance class appears to be necessary for Bug 349117's limited improvements, but the need for the change suggests a need to understand why this is impacting and consequently whether it constitutes an API change. Since the UML binding has no counterpart, is the Ecore value really necessary? It would be good to make it redundant so that we don't need to change it. -------- In general, there are some precision issues that cannot be determined without user assistance. Integer/Long/BigInteger does not need help, but for Double/BigDecimal it is not clear when growth is needed to give satisfactory precision for irrational numbers. The pivot evaluator therefore needs an evaluation option to select Double/BigDecimal. On 64 bit machines, an evaluation option could direct use of Long even though Integer is adequate. NB. java.lang.Long.hashCode() is an xor of 2 32 bit values on my 32 bit machine; does this change for 64 bits? If the Library instanceClass cannot be deduced or made redundant, it may be necessary to have an evaluation option to enable the otherwise API-breaking change.
(In reply to comment #0) > Continuing from Bug 342644 I've started by creating a few failing tests in EvaluationNumberOperationTest. See branch bug/344368. > Evaluation is mostly supported for Java's Long; the NFE comment in > NumberUtil.coerceNumber is wrong. ObjectUtil.isPrimitive enumerates the > supported types. I've remove the respective comment on the branch and added support to coerce a BigInteger to long if possible. > Evaluation support for BigDecimal and BigInteger is limited to > ObjectUtil.equal. If lucky, gratuitously big numbers can be coerced correctly > to Integer/Double, but the handling of BigInteger to Long is missing and the > requisite conversion is not certain to occur without a CCE. uggestions as to where to flag BigDecimal / BigInteger as an error? Already in the evaluate(...) call? We could try to coerce to long or int when receiving the object in evaluate(...) and if that fails throw an exception. What do you think? > Integer/Double overflows cause silent arithmetic malfunction. Integer does not > promote to Long. A sequence of calculations involving Long may CCE. > - bug: CollectionUtil.sum CCEs for Long. Fixed on branch. See test EvaluationNumberOperationTest.testSumOverLongInducedBySelf and (still failing because of the parser issue you mention) testSumOverLong. > Numbers must be coerced to Integer/Long/Double by the source, since numerous > Integer-only recipients do not convert. > - bug: PropertyCallExp does not coerce incoming numerics Sorry, don't understand. Can you explain a bit more or provide a failing test? > UML/EcoreEvaluationEnvironment.isTypeOf/isKindOf do not handle Long. They do on bug/349117. > UML/EcoreEvaluationEnvironment.isTypeOf uses getInstanceClass so changing > UnlimitedNatural to have a java.lang.Integer will change the behaviour of > Integer/UnlimitedNatural tests which previously reported different types > correctly but for the wrong reason, to report equal types incorrectly. Not sure I understand. Who changes UnlimitedNatural's implementation class? The change of Integer's implementation class to java.lang.Long didn't make any existing tests regarding UnlimitedNatural fail. Can you explain? Are you saying there may be incompatibilities due to the change of Integer's implementation class from java.lang.Integer to java.lang.Long with regard to comparing its type to UnlimitedNatural's type? Any test that would show what could go wrong? > Looking again at the 13 usages of getInstanceClass(). I think that just about > every one has an associated bug either for Java Integer/Long or OCL > Integer/UnlimitedNatural. Again, it'll help if you just commit the respective failing tests to this branch. > The usage of instanceTypeName for built-in types is a dangerous complexity. I > recommend eliminating all usages of getInstanceClass for OCL types. Retain only > those usages that extend from OCL to Ecore and user data types. The diverse > null-ness of oclstdlib.ecore instanceTypeName is then irrelevant. Feel free to give it a shot. I'll work on the parser problem with long values to make sure they can be parsed correctly.
(In reply to comment #1) > Long is not supported in the parser, because there is a programmed number range > trap and because IntegerLiteralExpCSImpl.integerSymbol is an Integer. Not even > the full 32 bit range is supported for UnlimitedNatural. I'll take a look. > Long is not supported in the AST, IntegerLiteralExp.integerSymbol is an > Integer. See above :-) > Long is not supported in the Library, in OCLStandardLibraryImpl there is > "OCL_INTEGER.setInstanceClass(Integer.class)". Not with bug/349117 where I changed that to Long.class already. > Long is substantially supported but almost totally untested in the evaluator. > oclKindOf and oclTypeOf lack support, and numeric growth fails: e.g. oclKindOf/oclTypeOf are fixed on bug/349117. > assertResult(2147483648L, "(-1-2147483647).abs()"); Fixed on bug/344368, see test EvaluationNumberOperationTest.testGrowingIntToLongOnSubtractionWithAbs > and a Fibonnaci series evaluation generates negative numbers after overflow. Do you have a test for this? PLUS/MINUS/TIMES already work based on long and then NumberUtil.coerceNumber. > Long is not consistently supported for property and tuple access; see Bug > 349117. Fixed there. > -------- > Parser and AST inadequacies can perhaps be fixed by introducing > LongLiteralExp*. Any API issue is hopefully limited to the 'permissible' > evolution of EMF Factory/Package. > > The evaluator can be tested and debugged for Long and the Long/Integer > boundary. Should not present an API issue. > > Field accesses can be fixed as in Bug 349117, and uncoerceable BigInteger and > BigDecimal should be trapped at source rather than allowed to fail at some > indeterminate point. BigInteger and BigDecimal are not supported by the Ecore > evaluator. Agreed, no guaranteed support for BigInteger / BigDecimal within the interpreter. Trying to coerce at entry in evaluate(...) may be okay, though. > Changing the library to have Long.class as the instance class appears to be > necessary for Bug 349117's limited improvements, but the need for the change > suggests a need to understand why this is impacting and consequently whether it > constitutes an API change. Since the UML binding has no counterpart, is the > Ecore value really necessary? It would be good to make it redundant so that we > don't need to change it. Where does our API specify this and how do clients depend on it? Doesn't an unchanged set of test verdicts demonstrate that there is no client-observable change? > -------- > In general, there are some precision issues that cannot be determined without > user assistance. Integer/Long/BigInteger does not need help, but for > Double/BigDecimal it is not clear when growth is needed to give satisfactory > precision for irrational numbers. > > The pivot evaluator therefore needs an evaluation option to select > Double/BigDecimal. > > On 64 bit machines, an evaluation option could direct use of Long even though > Integer is adequate. NB. java.lang.Long.hashCode() is an xor of 2 32 bit values > on my 32 bit machine; does this change for 64 bits? My JDK says "return (int)(value ^ (value >>> 32));" for Long.hashCode(). To me this looks like an xor between two long values which then is cast to an int. Should be the same, no matter how many bits the VM. > If the Library instanceClass cannot be deduced or made redundant, it may be > necessary to have an evaluation option to enable the otherwise API-breaking > change. Again, please explain why this is an API-breaking change.
Created attachment 199622 [details] Patch doing long instead of int where necessary As discussed. All tests green; merged bug/349117, so this patch also fixes 349117
(In reply to comment #1) > Parser and AST inadequacies can perhaps be fixed by introducing > LongLiteralExp*. Any API issue is hopefully limited to the 'permissible' > evolution of EMF Factory/Package. No chance. API compatibility is never that easy. For instance to add OppositePropertyCallExp we had to have a VisitorExtension. Here we have to extend both AST and CST and extended OCL languages for which the re-used parser currently guarantees to prohibit non-32 bit values. This cannot be fixed in a Service Release, since an SR1 AST read by an SR0 application could fail. If it has to wait for Juno, then a cleaner solution is to augment IntegerLiteral*.integerSymbol, with an extendedIntegerSymbol and an IntegerExtension interface that Long-aware clients can test for. To preserve compatibility, extendedIntegerSymbol should be unset when it is a sign-extension, therefore avoiding its unnecessary appearance in a serialized AST. In Juno, the default parsing behaviour can be Long-aware, but we need to provide a parsing option to retain 32-bit capability, and more importantly a Workspace and/or Project Preference setting so that a user of XOCL (unknown OCL extending project) can still have the front-end 32-bit check that XOCL had previously. (In reply to comment #4) > > and a Fibonnaci series evaluation generates negative numbers after overflow. > > Do you have a test for this? PLUS/MINUS/TIMES already work based on long and > then NumberUtil.coerceNumber. See Bug 341944. > > Changing the library to have Long.class as the instance class appears to be > > necessary for Bug 349117's limited improvements, but the need for the change > > suggests a need to understand why this is impacting and consequently whether it > > constitutes an API change. Since the UML binding has no counterpart, is the > > Ecore value really necessary? It would be good to make it redundant so that we > > don't need to change it. > > Where does our API specify this and how do clients depend on it? Doesn't an > unchanged set of test verdicts demonstrate that there is no client-observable > change? ... > > If the Library instanceClass cannot be deduced or made redundant, it may be > > necessary to have an evaluation option to enable the otherwise API-breaking > > change. > > Again, please explain why this is an API-breaking change. I do not understand what instanceClass does, but clearly it needs to change, so clearly it has some effect, and so users might detect it. As noted above, it is not needed for UML, so I think the logic that uses instanceClass can be improved to tolerate the existing value and so avoid the risk. Until this is understood it is difficult to make any real plans.
(In reply to comment #6) I see your point regarding AST compatibility and will try the suggested fix with an extended interface for long access. > OppositePropertyCallExp we had to have a VisitorExtension. Here we have to > extend both AST and CST and extended OCL languages for which the re-used parser > currently guarantees to prohibit non-32 bit values. This cannot be fixed in a > Service Release, since an SR1 AST read by an SR0 application could fail. What is the deal with prohibiting values longer than 32 bits? > > If it has to wait for Juno, then a cleaner solution is to augment > IntegerLiteral*.integerSymbol, with an extendedIntegerSymbol and an > IntegerExtension interface that Long-aware clients can test for. To preserve > compatibility, extendedIntegerSymbol should be unset when it is a > sign-extension, therefore avoiding its unnecessary appearance in a serialized > AST. > > In Juno, the default parsing behaviour can be Long-aware, but we need to > provide a parsing option to retain 32-bit capability, and more importantly a > Workspace and/or Project Preference setting so that a user of XOCL (unknown OCL > extending project) can still have the front-end 32-bit check that XOCL had > previously. > > (In reply to comment #4) > > > and a Fibonnaci series evaluation generates negative numbers after overflow. > > > > Do you have a test for this? PLUS/MINUS/TIMES already work based on long and > > then NumberUtil.coerceNumber. > > See Bug 341944. Ok, will take a look based on the Fibonacci test. Is it possible that it blows the long value range too? > > > > Changing the library to have Long.class as the instance class appears to be > > > necessary for Bug 349117's limited improvements, but the need for the change > > > suggests a need to understand why this is impacting and consequently whether it > > > constitutes an API change. Since the UML binding has no counterpart, is the > > > Ecore value really necessary? It would be good to make it redundant so that we > > > don't need to change it. > > > > Where does our API specify this and how do clients depend on it? Doesn't an > > unchanged set of test verdicts demonstrate that there is no client-observable > > change? > ... > > > If the Library instanceClass cannot be deduced or made redundant, it may be > > > necessary to have an evaluation option to enable the otherwise API-breaking > > > change. > > > > Again, please explain why this is an API-breaking change. > > I do not understand what instanceClass does The instanceClass thing seems trickier. If I understand it correctly, it's used for instanceof checks and the Tupperware creation. I don't think it can easily be made redundant., but clearly it needs to change, so > clearly it has some effect, and so users might detect it. > > As noted above, it is not needed for UML, so I think the logic that uses > instanceClass can be improved to tolerate the existing value and so avoid the > risk. Until this is understood it is difficult to make any real plans.
Tuple, not Tupperware... that was android, sorry.
(In reply to comment #7) > What is the deal with prohibiting values longer than 32 bits? If I write an XOCL program using the literal "9876543210" the Indigo OCL parser detects an error and so there is no need for XOCL tooling to be Long-aware. If the OCL parser changes, XOCL tooling must then handle Long correctly, which may not happen if the XOCL developers are insufficiently active, so we need to be able to allow a user to select parser compatibility. > Ok, will take a look based on the Fibonacci test. Is it possible that it blows > the long value range too? Certainly. It is an infinite series. It may be that this Long-change is sufficiently painful that you decide to do BigInteger and perhaps BigDecimal too.
(In reply to comment #9) > (In reply to comment #7) > > What is the deal with prohibiting values longer than 32 bits? > > If I write an XOCL program using the literal "9876543210" the Indigo OCL parser > detects an error and so there is no need for XOCL tooling to be Long-aware. > > If the OCL parser changes, XOCL tooling must then handle Long correctly, which > may not happen if the XOCL developers are insufficiently active, so we need to > be able to allow a user to select parser compatibility. Strange, but ok. Then, what's a good way to allow a user to make the selection? > > Ok, will take a look based on the Fibonacci test. Is it possible that it blows > > the long value range too? > > Certainly. It is an infinite series. It may be that this Long-change is > sufficiently painful that you decide to do BigInteger and perhaps BigDecimal > too. I have fairly limited time working on this right now. I'm really driven by our internal customers who basically only had this issue with tuples. If getting this fixed entails fixing 344368 then so be it.
Created attachment 199648 [details] Using LongLiteralExp as subtype of IntegerLiteralExp This leaves existing ASTs unchanged. The patch introduces the new type LongLiteralExp which extends IntegerLiteralExp. When the analyzer detects a literal that cannot be coerced into an int, it creates a LongLiteralExp instead of an IntegerLiteralExp and assigns the symbol to the Long-typed attribute. See branch bug/344368.
Collision: ... (In reply to comment #6) > If it has to wait for Juno, then a cleaner solution is to augment > IntegerLiteral*.integerSymbol, with an extendedIntegerSymbol and an > IntegerExtension interface that Long-aware clients can test for. To preserve > compatibility, extendedIntegerSymbol should be unset when it is a > sign-extension, therefore avoiding its unnecessary appearance in a serialized > AST. which only works for Long and needs care with notification/eGet/eSet. It might be better to reimplement IntegerLiteral*.integerSymbol using java.lang.Number taking care to "@generate NOT" so that - changes are still notified by the same Package literal for API compatibility - coerced values are in any notification for API compatibility - getIntegerSymbol()/setIntegerSymbol() use Integer for API compatibility - getLongSymbol()/setLongSymbol() use Long [- getBigIntegerSymbol()/setBigIntegerSymbol() use BigInteger] - getNumberSymbol()/setNumberSymbol() use Number - eGet uses coerced values for API compatibility - eSet accepts a variety of values - Factory conversion works between ASCII and a variety of number types
Created attachment 199654 [details] Worked around API compatibility problems Added compatibility filters, mostly for additional generated or re-numbered constants and a few methods inside the evaluator which now take long instead of int arguments which I don't consider part of the OCL component's external contract.
(In reply to comment #9) > (In reply to comment #7) > > What is the deal with prohibiting values longer than 32 bits? > > If I write an XOCL program using the literal "9876543210" the Indigo OCL parser > detects an error and so there is no need for XOCL tooling to be Long-aware. > > If the OCL parser changes, XOCL tooling must then handle Long correctly, which > may not happen if the XOCL developers are insufficiently active, so we need to > be able to allow a user to select parser compatibility. After reading the comments on this page and reviewing the changes Axel made, it seems all that is missing to fix this issue is a way to switch between the default "interger implementation" and the modified "long aware version" in order to avoid changing behaviour other tools may rely on. Since OCL already appears in the preferences menu, I suggest adding a respective option to the preferences that enables the user to activate long-awareness if needed. If you agree to that, I'll try to come up with a patch to realize that.
(In reply to comment #14) > If you agree to that, I'll try to come up with a patch to realize that. Sounds promising. This patch is certainly nearly there; we just have to be paranoid about long-standing compatibility given the 'stability' of customers such as QVTo and GMF. Have a look at Bug 360354 and the corresponding GIT branch that introduces a new ocl.ui plugin with combined preference/property pages. This could be a good home for new preference controls; just need to ensure that a minimal compatible non-GUI capability is available to existing standalone users. We can package an additional plugin in the existing runtime feature, but we cannot add any extra outgoing dependencies to Eclipse UI. Perhaps this means two new plugins; ...ocl.preferences and ocl.preferences.ui. I would like the ...ocl.preferences to provide extensible services to any of the Ecore/UML/Pivot bindings without depending on any of them. Perhaps an ...ocl.preference extension point. [The layout/content of preference/property pages is TBD. The layout in the branch is just a prototype demonstration of principle.]
Martin, I'm all for it. Ed?
bug/344368 branch has extended integer support for IntegerLiteralExpCS, IntegerLiteralExp and Tuples. Various maths corner cases are fixed. Common tests apply to Ecore and UML bindings.
Looks good and works well. +1
Pushed to master.
And resolved.
CLOSED after a year in the RESOLVED state.
See Bug 475907 for a fix for the persistence of the extendedIntegerSymbol.
(In reply to Ed Willink from comment #22) > See Bug 475907 for a fix for the persistence of the extendedIntegerSymbol. Bug 475907 is a QVTo fudge. Bug 488278 is the OCL fix.
