Bug 64394 - Any >>= operator and typelib_TypeClass_ENUM
Summary: Any >>= operator and typelib_TypeClass_ENUM
Status: RESOLVED NOTABUG
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: sdk (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium enhancement
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 14:26 UTC by renatomauro
Modified: 2015-01-19 08:44 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description renatomauro 2013-05-09 14:26:32 UTC
Is it possible, in future, to add in /cppu/inc/com/sun/star/uno/Any.hxx

the case for enums at each occurrence of "case typelib_TypeClass_LONG", e.g.

add 

case typelib_TypeClass_LONG:
case typelib_TypeClass_ENUM:           // NEW CASE
case typelib_TypeClass_UNSIGNED_LONG:
value = * reinterpret_cast< const sal_Int32 * >( rAny.pData );

to

inline sal_Bool SAL_CALL operator >>= ( const Any & rAny, sal_Int32 & value ) SAL_THROW( () )

?


Thank you very much.
Comment 1 Stephan Bergmann 2014-01-16 15:56:57 UTC
What is a problem you would want to solve with that change?

For type safety, it is generally better to support as few implicit conversions as possible (and we already support too many in the com::sun::star::uno::Any functionality, for my taste).
Comment 2 Robinson Tryon (qubit) 2014-01-17 01:11:07 UTC
(In reply to comment #1)
> What is a problem you would want to solve with that change?
> 
> For type safety, it is generally better to support as few implicit
> conversions as possible (and we already support too many in the
> com::sun::star::uno::Any functionality, for my taste).

Status -> NEEDINFO

Renatomauro, please change the status back to UNCONFIRMED after you've responded to sbergman's questions.

Thanks!
Comment 3 renatomauro 2014-01-21 22:45:54 UTC
(In reply to comment #1)
> What is a problem you would want to solve with that change?
> 
> For type safety, it is generally better to support as few implicit
> conversions as possible (and we already support too many in the
> com::sun::star::uno::Any functionality, for my taste).

Generally speaking, I agree. In practice: having a wrapper for the Any type and template methods forbidden, there are many methods, such as ToInt32(), ToInt64(), ToBool(), ToUInt32() and so on. For each of them the code has avoidable "if" and reinterpret_cast, if an implicit conversion for enum would exist:

[...]
  *m_pAny >>= Value ;
  // hack to manage typelib_TypeClass_ENUM too - BEGIN
  if ( m_pAny->pType->eTypeClass == typelib_TypeClass_ENUM )
  {
      Value = *reinterpret_cast<const sal_Int32*>(m_pAny->pData);
  }
  // hack to manage typelib_TypeClass_ENUM too - END
[...]

Ok, for the custom type (e.g. Size) a cast is mandatory and convert it to Int32 or Bool makes no sense; instead enum is a common base type and it is very easy to be converted to other numerical base types.
Comment 4 Robinson Tryon (qubit) 2015-01-14 08:16:19 UTC
Stephan: Where are we with this report?
Comment 5 Stephan Bergmann 2015-01-14 08:31:53 UTC
Sorry, had completely lost sight of this issue.

But I'm still not sure about any benefit of weakening the semantics of Any::operator>>=.  If you have an Any instance A that is supposed to contain a value of UNO enum type E, the way to extract it is either

  E e; if (A >>= e) ...

or

  A.get<E>()

Not sure what ToInt32() etc. functions you are talking about.  (I assume you are talking about a set of functions designed to extract values from an Any in a "maximally forgiving" way, coercing values from "wrong" types into values of the "right" type in creative ways.  While there may be places where such functions are useful, their weak semantics are not useful for general functionality like Any::operator>>=.)
Comment 6 tommy27 2015-01-18 10:58:49 UTC
@renatomauro
please answer Stephan comments and tell if you agree with him (and we may close this report) or if you have other arguments to support your idea.

I set status to NEEDINFO
Comment 7 renatomauro 2015-01-18 13:58:03 UTC
About the >>= operator of the Any class, using its overloaded method 

inline sal_Bool SAL_CALL operator >>= ( const Any & rAny, sal_Int32 & value ) SAL_THROW( () )

the user's requirement is: "please, convert to sal_Int32 as far as this is possible"; so the user is not asking to retrieve the enum value as an enum type.
 
It seems that today the >>= operator implementation is definitely consistent about the type dimension, i.e. a type is never converted to a smaller sized type and, in this case, the operator returns false. This is a good pattern and, for this overload, the reasonable input, as now, are the following typelib_TypeClass_* types: BYTE, SHORT, UNSIGNED_SHORT, LONG, UNSIGNED_LONG.
So, the weakeness is already accepted about the signed/unsigned known issues.
Now, what about typelib_TypeClass_ENUM? What is its size? For sure it is less than or equal to the bit depth of the processor, or, better, the bit depth of the binary. So, nowadays at least 
inline sal_Bool SAL_CALL operator >>= ( const Any & rAny, sal_Int64 & value ) SAL_THROW( () )
and
inline sal_Bool SAL_CALL operator >>= ( const Any & rAny, sal_uInt64 & value ) SAL_THROW( () )
could safely handle typelib_TypeClass_ENUM.
And, using some size check (right now I cannot say at compile-time or run-time),  sal_Int32/sal_uInt32 and whatever destination integer/float/double type could too.

The idea is: as far as possible/reasonable, better doing validity checks, in the callee code rather than in the caller code on the returned value.

Otherwise, as it is today, the user's code is interleaved not only by the check about the operator return value (true/false), but also, if false, by the check if it is a typelib_TypeClass_ENUM to then make the conversion by his/her-self.

But, I must admit, the word "reasonable" is always opened to personal opinions, in turn reasonable and, for sure, interesting.

Thanks for time.
Comment 8 Stephan Bergmann 2015-01-19 08:44:24 UTC
(In reply to renatomauro from comment #7)
> It seems that today the >>= operator implementation is definitely consistent
> about the type dimension, i.e. a type is never converted to a smaller sized
> type and, in this case, the operator returns false. This is a good pattern
> and, for this overload, the reasonable input, as now, are the following
> typelib_TypeClass_* types: BYTE, SHORT, UNSIGNED_SHORT, LONG, UNSIGNED_LONG.
> So, the weakeness is already accepted about the signed/unsigned known issues.

For historic reasons, Any::operator >>= for numeric types (and for numeric types only) employs some specific conversions that could be characterized as "widening modulo signedness."  Whether or not this would be considered reasonable is somewhat irrelevant, given how much (internal and external) code has been written against that specific behaviour over time---it's just the way it is and practically impossible to change anyway.

> Now, what about typelib_TypeClass_ENUM? What is its size? For sure it is
> less than or equal to the bit depth of the processor, or, better, the bit

See <http://www.openoffice.org/udk/common/man/typesystem.html> for what an enum's members are.  But the important thing is that an enum type is not a numeric type, as far as UNO is concerned, and conversion from the (numeric) value of an enum type's member to a numeric type is never considered by Any::operator >>=, for better or worse.  (Just like it never considers conversion from boolean values, or from values of a struct type with a single numeric member field, or...)

> The idea is: as far as possible/reasonable, better doing validity checks, in
> the callee code rather than in the caller code on the returned value.

That is not necessarily a good idea.  These are not "validity checks" but changes of semantics.

> Otherwise, as it is today, the user's code is interleaved not only by the
> check about the operator return value (true/false), but also, if false, by
> the check if it is a typelib_TypeClass_ENUM to then make the conversion by
> his/her-self.

Code that legitimately wants to extract from an any an enum value as an integer value is probably very rare.

I'm closing this as NOTABUG.  If you still see need for discussion, please move it to the mailing list, which is a better place for discussions like this.