Bug 206808

Comment 1 Christian Damus 2007-11-06 23:05:24 EST

Created attachment 82297 [details]
Proposed J2SE 5.0-ification

Attached a proposed update of the API to adopt J2SE 5.0 language constructs.

Comment 2 Christian Damus 2007-11-08 19:43:15 EST

Created attachment 82505 [details]
Re-synchronized patch

Attached an updated patch for changes recently committed to HEAD.

Comment 3 Christian Damus 2007-11-08 19:54:53 EST

Hi, Ed, Anthony,

Would you mind having a look at the attached patch when you get a chance?  I am most concerned about the org.eclipse.emf.validation plug-in (the generic EvaluationMode- and IValidator-related API, in particular).

Of course, any ideas that you might have for further exploitation of J2SE 5.0 constructs will be most welcome.

TIA!

Comment 4 Anthony Hunter 2007-11-13 19:14:54 EST

I could not see any issues with your proposed changes.

The generic EvaluationMode- and IValidator-related API made sense to me, in my limited knowledge of generics.

Comment 5 Christian Damus 2007-11-14 08:30:51 EST

Created attachment 82865 [details]
Updates from review comments

Attached an updated patch to address review comments (thanks Ed and Anthony).

  - use Boolean.valueOf(String) instead of
    Boolean.TRUE.toString().equalsIgnoreCase(String)
    in Tracing::shouldTrace(String) methods

  - use List<? super IStatus> for flexibility in
    AbstractValidator::evaluateConstraints(...)

  - declare IProviderOperation interface with type parameter
    <T extends Collection<? extends IModelConstraint>>
    instead of simply <T>

  - declare IProviderOperationExecutor interface as non-generic
    and make the execute(...) method generic, instead:
       <T> T execute(IProviderOperation<? extends T> op)
    (return type changes from void, but this is an internal API;
    should have been thus before)

  - updated AbstractValidator::AggregateStatus::getMaximalSeverity()
    to only check for the maximum in the "if" test within the loop
    body (and added comment to explain why CANCEL is ignored)

  - updated AbstractValidator::acceptConstraint(...) to specify
    IConstraintFilter as the iterator variable type

  - updated signatures of ClientContextManager::getBindings(...)
    methods to make them generic:
      <T extends IModelConstraint> Collection<T> getBindings(...,
             Collection<? extends T> constraints)

  - changed ClientContextManager::defaultContexts to type
    Set<ClientContext> from Set<IClientContext>

I thought a good deal about the comments to declare operation return types of collection values as Collection<? extends Xyz> instead of simply Collection<Xyz> where the collections are read-only and decided against.  Employing Map<?, ?> for options is one thing, because it looks clean, but otherwise I hesitate to use the type system to enforce writability, which is an entrinsic property of collections that clients cannot work around by casting.

Comment 6 Christian Damus 2007-11-14 13:05:11 EST

The patch is committed to CVS with the following additional changes:

  - "Code Clean-up" to add missing @Override and @Deprecated annotations
  - update feature.xml plug-in dependencies

Comment 7 Christian Damus 2007-11-14 15:29:09 EST

Fixed in the EMF VALIDATION 1.2.0 I200711141453 build.

Comment 8 Nick Boldt 2008-01-28 16:35:49 EST

Move to verified as per bug 206558.