Bugzilla – Full Text Bug Listing
| Summary: | [library] Add tokenize etc | ||
|---|---|---|---|
| Product: | [Modeling] OCL | Reporter: | Ed Willink <ed> |
| Component: | Core | Assignee: | OCL Inbox <mdt-ocl-inbox> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | adolfosbh, eclipse |
| Version: | 3.2.0 | ||
| Target Milestone: | M7 | ||
| Hardware: | PC | ||
| OS: | Windows Vista | ||
| Whiteboard: | |||
|
Description
Ed Willink
Additional functions in bug/373289 branch. It looks like that bug/373289 mixes this bug and Bug 377374 I have only a couple of comments concerning this bug, that is, changes regarding the new library operations: o.e.o.EvaluationVisitorImpl: line 494: Should not return a Boolean object ? line 1339: Why SelectByType doesn't check that the object of a collection is null ?... SelectByKind does o.e.o.ecire.internal.OCLStandardLibraryImpl line 99: Could you give/add some explanation/documentation about this change ?. Apart from that +1 for the branch. Cheers, Adolfo. (In reply to comment #2) > o.e.o.EvaluationVisitorImpl: > > line 494: Should not return a Boolean object ? Line numbers have evolved. If this is TO_BOOLEAN the default Java boxing gives a Boolean. > line 1339: Why SelectByType doesn't check that the object of a collection is > null ?... SelectByKind does SelectByType is an exact match so the oclIsType(null, ...) will fail. SelectByKind speciically excludes null; a pragmatic decision; who wants to select the nulls, that conform to everything? > > o.e.o.ecire.internal.OCLStandardLibraryImpl > > line 99: Could you give/add some explanation/documentation about this change ?. The toString overload cures the long standing problem that any error message wirth 'invalid' in has EMF default mubo jum with name="OclInvalid_Class) etc. Since the public API is only EObject singleton, we can change the EObject class. b589f6ab9e2768d3c1d7f53ed49a85a736cccc1d looks OK. The test is provided by a2c42649c02e6d00437a6a94b6507b480ce66255 (a little hard to find because it's in testNumberToString()). d28289c56e612ee9c6fb3227e4e095ff458acfc7 looks good and has the proper test support. Model changes are additive only, and there is no genmodel and no generated Java interfaces for it, so additions to the ecore are not a compatibility issue. ee2b929635f28f3943f30ba8d3203dddb424c691 slightly changes the test suite structure. From what I can see, AbstractTestSuite now aggregates the tests formerly in AbstractEvaluationTest which was renamed to GenericEvaluationTestSuite. Other test classes were renamed into Generic... with specific subclasses introduced. Looks good. a2c42649c02e6d00437a6a94b6507b480ce66255 has a tiny redundancy for invalid's toString() which was already provided by b589f6ab9e2768d3c1d7f53ed49a85a736cccc1d. The commit unfortunately has vast whitespace changes for oclstdlib.ecore, but ignoring those the diff becomes readable. Additions of toString() in oclstdlib.ecore again provide no compatibility issues because it's not genmodeled. c1d03670564c5004789af688e6e657d54af17080 looks good. fa1861a7429f3aacc1288e56c8da66b4558f7af0 introduces commonPrecisionNumber. If number has a limited integral numerical type (Integer or Long) and referenceNumber is of type BigDecimal, Number.doubleValue() is applied to number which may introduce unnecessary inaccuracies because BigDecimal can exactly represent any Integer or Long value. I suggest treating the cases where number is an Integer or a Long special. Ah, I see: f40fdab23117a69c168451be4b5d9a7b4ff7503d introduces the selectByType/selectByKind to the ecore binding, relevant for IA. They are not provided as iterators but as operations on Collection(T). The IA therefore needs to provide special support for these operation calls on collections. I'll work on a corresponding patch and test cases later today. In 907aa134a7cdc290e216d470a9949f1b9e11ca5b, the oclAsSet conversions look OK to me. The evaluator converts invalid->oclAsSet() to invalid which seems right. null->oclAsSet() resolves to an empty set, all other objects are added as the single element to the set. I'd say this is the intended behavior. In ea22f6efbd33d7db8d23d9788d12f1b9bcbc493f there seems to be a merge conflict in .api_filters which is then removed in dbbed366f1485bc13e0332af3895b0b59664bec0, so that's ok. I don't know the LPG code, hence cannot provide a quality review here. The fact that all tests are still OK suggests to me that the changes around import handling are OK. In 22a156c8660d2e3b28e21c5374ae95e4dc5d4585 I don't understand the changes in ModelWithErrors.ecore. Why did you have to insert 999 as argument to oclIsInvalid()? Why the Integer in DelegatesTest.java? Before agreeing I'd like to get the IA patch done and would like to understand the oclIsInvalid changes in the tests. Also, the precision fix mentioned above for the numeric coercion should be added. I've prepared a patch already which I'll commit to this branch later. (In reply to comment #4) Thanks > fa1861a7429f3aacc1288e56c8da66b4558f7af0 introduces commonPrecisionNumber. If > number has a limited integral numerical type (Integer or Long) and > referenceNumber is of type BigDecimal, Number.doubleValue() is applied to > number which may introduce unnecessary inaccuracies because BigDecimal can > exactly represent any Integer or Long value. I suggest treating the cases where > number is an Integer or a Long special. } else if (number instanceof Long) { return BigDecimal.valueOf(number.longValue()); } else if ((number instanceof Integer) || (number instanceof Short) || (number instanceof Byte)) { return BigDecimal.valueOf(number.intValue()); > oclAsSet .... I'd say this is the intended behavior. This is only provided for completeness; the pivot model uses it as its AST for anObject->.... With the library operation present we have the option to start using it in the legacy AST synthesis, moving the magic and fractionally wrong run-time type nasties to compile-time. > In 22a156c8660d2e3b28e21c5374ae95e4dc5d4585 I don't understand the changes in > ModelWithErrors.ecore. Why did you have to insert 999 as argument to > oclIsInvalid()? Why the Integer in DelegatesTest.java? The problem was that the previous semantic error was no longer an error, since aCollection.oclIsInvalid() is no longer an unresolved operation. The "999" makes it a semantic error again. > > Before agreeing I'd like to get the IA patch done and would like to understand > the oclIsInvalid changes in the tests. Also, the precision fix mentioned above > for the numeric coercion should be added. I've prepared a patch already which > I'll commit to this branch later. >> fa1861a7429f3aacc1288e56c8da66b4558f7af0 introduces commonPrecisionNumber. If >> number has a limited integral numerical type (Integer or Long) and >> referenceNumber is of type BigDecimal, Number.doubleValue() is applied to >> number which may introduce unnecessary inaccuracies because BigDecimal can >> exactly represent any Integer or Long value. I suggest treating the cases where >> number is an Integer or a Long special. > > } else if (number instanceof Long) { > return BigDecimal.valueOf(number.longValue()); > } else if ((number instanceof Integer) > || (number instanceof Short) || (number instanceof Byte)) { > return BigDecimal.valueOf(number.intValue()); Great. In my patch I missed the Short/Byte cases. Thanks. I'll add them. I think longValue() works well for all four (Long, Integer, Short, Byte) without running the risk of rounding errors. >> oclAsSet .... I'd say this is the intended behavior. > > This is only provided for completeness; the pivot model uses it as its AST for > anObject->.... With the library operation present we have the option to start > using it in the legacy AST synthesis, moving the magic and fractionally wrong > run-time type nasties to compile-time. Thanks. >> In 22a156c8660d2e3b28e21c5374ae95e4dc5d4585 I don't understand the changes in >> ModelWithErrors.ecore. Why did you have to insert 999 as argument to >> oclIsInvalid()? Why the Integer in DelegatesTest.java? > > The problem was that the previous semantic error was no longer an error, since > aCollection.oclIsInvalid() is no longer an unresolved operation. The "999" > makes it a semantic error again. Understood. I've pushed a fix for the IA's traceback variant which handles selectByType and selectByKind, including a few tests. I'm yet to add support for it in the navigation step variant. I think that between you, each contribution to bug/344368b was +1'd so I've pushed to master and it's building. Apologies if I jumped the gun on some discussion. Thanks for the priomnpt reviews so that we're no so lasrt minute on M7. AFAIK - there are some IA corrolaries to sort out; no problem in examples. there are some tests to consider rescuing from the original bug/349117 branch. > I think that between you, each contribution to bug/344368b was +1'd so I've > pushed to master and it's building. Apologies if I jumped the gun on some > discussion. Thanks for the priomnpt reviews so that we're no so lasrt minute on > M7. Correct. > AFAIK - > > there are some IA corrolaries to sort out; no problem in examples. Yes. Working on it on the bug/373289 branch. > there are some tests to consider rescuing from the original bug/349117 branch. Yes. You may want to look at that again. Hi guys, I've received access one hour ago. I'm pleased you both are working on the reviews. Thanks, Adolfo. Pushed to master. Additional changes for IA pushed to branch. Ed, do you want to cherry-pick it into master or do you want me to do that? Your patch to examples; no review needed (I hope). You cherrypick, which seems to look better in the history thyan tangled merges. Pushed corresponding IA changes, cherry-picked from bug/373289 branch, to master. (In reply to comment #14) > Pushed corresponding IA changes, cherry-picked from bug/373289 branch, to > master. Is the branch still needed? No, it can be archived. (In reply to comment #16) > No, it can be archived. But it was just successful work in progress and has no significant uncommitted work, so why archive it? Ah, sorry, I misunderstood the archiving policy. In that case, it can be deleted. CLOSED after a year in the RESOLVED state. |