search

eclipse-ocl / org.eclipse.ocl

Eclipse Public License 2.0
0 stars 0 forks source link

[pivot] Replace all created BasicEList by BasicInternalEList #1143

Closed eclipse-ocl-bot closed 1 month ago

eclipse-ocl-bot commented 1 month ago

| --- | --- | | Bugzilla Link | 412690 | | Status | CLOSED FIXED | | Importance | P3 critical | | Reported | Jul 10, 2013 11:34 EDT | | Modified | May 25, 2015 17:18 EDT | | Reporter | Ed Willink |

Description

Although the declared EMF list-returning API is EList, the actual API is InternalEList, so to avoid CCEs wherever the actual API is used, the Pivot evaluation must return BasicInternalEList rather than BasicEList.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 10, 2013 12:27

Fix pushed to branch edw/412690 for now and to edw/410682sr (the 'a' candidate).

eclipse-ocl-bot commented 1 month ago

By Ed Merks on Jul 10, 2013 13:13

Note that even BasicInternalEList doesn't implement EStructuralFeature.Setting so org.eclipse.emf.ecore.impl.BasicEObjectImpl.eSetting(EStructuralFeature) would also result in a CCE; something you'll notice if you try to use a cross referencer on the model...

If the list is intended to be unmodifiable, consider using org.eclipse.emf.ecore.util.EcoreEList.UnmodifiableEList.UnmodifiableEList(InternalEObject, EStructuralFeature, int, Object[]) instead.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 11, 2013 02:01

Indeed. A cross-referencing JUnit test fails.

Updated fix pushed to branch edw/412690 and to edw/410682sr.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 11, 2013 08:55

Pushed to master for M1. 4.2.0 N-build available on http://www.eclipse.org/modeling/mdt/downloads/?project=ocl.

commit 65dc2e291695bc0466eb93377fe7b4e8fda05826\ commit b33d87aa2eb93a61373902ecd460dc33b17a9d2d

SR fix still pending.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Jul 15, 2013 07:03

It looks like a convenient refactoring

However:

  1. I don't have a clear composition about differences between Value.asObject() and Value.asEcoreObject(). Rather than studying every Value impl, I prefer to hear from your feedback.
  2. Refactoring focuses on creating ELists via ValueUtil.createEList, centralizing EList constructions. Good.
  3. However, those EList construction methods focus on using the value.asEcoreObject() method for Values, whereas EInvokeOperation changes were indirectly using the value.asObject() one (via the ValuUtil.asObject()) method.
eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 19, 2013 00:17

(In reply to comment #5)

Value.asObject() and Value.asEcoreObject().

When everything was boxed as a Value, it was easy, asEcoreObject unboxed it to give the conventional Classic Ecore OCL API.

The CG highlighted that Boxed values were very costly for EObject, so EObject, Boolean, String are not boxed. I'm looking for a stronger way to clarify the API thatn just boxedValue/unboxedValue as the parameter name.

asEcoreObject still gives the Classic Ecore OCL value. Unfortuantely I've had to add an IdREsolver to help out with EnumerationLiterals.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Jul 26, 2013 12:02

Fix already on master.

SR abandoned in favour of an early 4.2.0 that will be Kepler installable.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 16, 2013 11:38

(In reply to comment #7)

Fix already on master.

SR abandoned in favour of an early 4.2.0 that will be Kepler installable.

SR required after all. Please complete review after all.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 16, 2013 14:39

Trying to study this again....

Unfortunately, I don't follow your comment 6 ...

  1. However, those EList construction methods focus on using the value.asEcoreObject() method for Values, whereas EInvokeOperation changes were indirectly using the value.asObject() one (via the ValuUtil.asObject()) method.

This has not been explicitly considered. To be more specific...

Before the refactoring, for instance in EInvokeOperation, line 81:

BasicEList arguments = new BasicInternalEList(Object.class);\ arguments.add(asObject(argumentValue));

Which it turns into adding argumentValue.asObject() to the arguments

After the refactoring:

EList arguments = ValuesUtil.createEList(argumentValue);

Which it turns into adding argumentValue.asEcoreObject() to the arguments

This is a clearly a potentially harmful change in the refactoring. To be a correct, asObject() should return the same object as calling asEcoreObject() for every Value impl.

Are you aware of this change?. If so, I can either trust in test cases or review every Value impl to verify that. Options:\ a) If you were not aware of this "slight" change. Please, fix the refactoring or study that for every Value impl, calling the .asObject() we have the same result than .asEcoreObject()\ b) If you aware of this "slight" change. I'll review every ValueImpl during the weekend/monday.

I hope this makes sense.

Regards,\ Adolfo.\ Regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 17, 2013 02:27

An EList is used where Ecore objects are required, there asEcoreObject is appropriate. asEcoreObject is idemPotent so can be applied multiple times without harm.

In so far as extra asEcoreObject calls are concerned there is a faint possibility of a bug fix, but since the JUnit tests are fairly comprehensive that is unlikely.

But in order to make progress I'll remove createEList and loop in each caller.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 17, 2013 04:00

Convenient refactoring removed. Much simpler patch to review as branch edw/412690sr

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 17, 2013 05:49

+1 for having branch edw/412690 merged into maintehance/R4_1 branch

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 17, 2013 05:49

(In reply to comment #12)

having branch edw/412690

I meant edw/412690sr

Regards,\ Adolfo.

eclipse-ocl-bot commented 1 month ago

By Adolfo Sanchez-Barbudo Herrera on Aug 17, 2013 05:51

Activating the review flag, since it was requested for the maintenance branch changes review

eclipse-ocl-bot commented 1 month ago

By Ed Willink on Aug 17, 2013 12:06

Thanks. Pushed to maintenance/R4_1 for Kepler SR1 RC1.

eclipse-ocl-bot commented 1 month ago

By Ed Willink on May 25, 2015 17:18

CLOSED after more than a year in the RESOLVED state.

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.