Dmitry Eremin-Solenikov(lumag) replied on github web page:
test/validation/api/pktio/pktio.c
line 16
@@ -1195,10 +1195,8 @@ void pktio_test_index(void)
ndx = odp_pktio_index(pktio);
CU_ASSERT(ndx >= 0);
- CU_ASSERT(odp_pktio_index(pktio_inval) < 0);
CU_ASSERT(odp_pktio_close(pktio) == 0);
- CU_ASSERT(odp_pktio_index(pktio) < 0);
Comment:
Generally I would agree. However we explicitly have the invalid usecase in API:
```
@retval <0 On failure (e.g., handle not valid)
```
> Dmitry Eremin-Solenikov(lumag) wrote:
> Is there a reason to move function bodies?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Maybe `(uintptr_t)(handle) == (uintptr_t)0` would do the trick?
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Very strange. I'm Ok with this workaround, but a comment about clang
>>> version might be appropriate. Don't know if it's worth doing a clang
>>> version check. We do things like that in the `ODP_STATIC_ASSERT()` macro
>>> itself for similar reasons.
>>>> Petri Savolainen(psavol) wrote:
>>>> odp_packet.c:52:19: error: static_assert expression is not an integral
>>>> constant
>>>> expression
>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_BUFFER_INVALID) == 0, "Buffer inval not
>>>> 0");
>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:30:
>>>> note:
>>>> expanded from macro '_odp_typeval'
>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>> ^
>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded from macro
>>>> 'ODP_STATIC_ASSERT'
>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>> ^~~~
>>>> odp_packet.c:52:19: note: cast that performs the conversions of a
>>>> reinterpret_cast is not allowed in a constant expression
>>>> ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:41:
>>>> note:
>>>> expanded from macro '_odp_typeval'
>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>> Petri Savolainen(psavol) wrote:
>>>>> static_assert() seems special. The clang version in Travis does not
>>>>> accept type casts with static_assert(). The cast confuses it to think
>>>>> that (uintptr_t)0 == 0 is not an integral constant.
>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>> `_odp_typeval()` is defined as:
>>>>>> ```
>>>>>> #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))
>>>>>> ```
>>>>>> It's used elsewhere in the ODP code and clang doesn't have any problem
>>>>>> with it. I've verified this works for clang 4.2.1. Since it works
>>>>>> elsewhere on Travis I'm not sure why it wouldn't in this instance.
>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>> I'm OK with just a modified commit name. I agree we needn't be too
>>>>>>> granular about these things.
>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>> I could rename the patch to:
>>>>>>>> "linux-gen: packet: use inlined flow hash and ts set"
>>>>>>>>
>>>>>>>> ... if necessary. The comment says already that also ts set is
>>>>>>>> modified the same way.
>>>>>>>>> Petri Savolainen(psavol) wrote:
>>>>>>>>> I did try several combinations of type casts of invalid values, also
>>>>>>>>> cast to uintptr_t. The problem is that gcc and clang (clang version
>>>>>>>>> 3.8.0-2ubuntu4) of my machine accepted all of those casts, but the
>>>>>>>>> clang version in Travis (clang version 3.8.0-2ubuntu3~trusty5) does
>>>>>>>>> not accept anything.
>>>>>>>>>
>>>>>>>>> Maybe it's a bug in the particular version of clang. Anyway, addition
>>>>>>>>> of this check even only on gcc side is improvement against what we
>>>>>>>>> currently have (no check). Packet invalid is currently 0xfffffff in
>>>>>>>>> ABI spec, but event/buffer invalid are 0.
>>>>>>>>>
>>>>>>>>> odp_packet.c:55:19: error: static_assert expression is not an
>>>>>>>>> integral constant
>>>>>>>>> expression
>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>> invalid not 0");
>>>>>>>>> ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>>>> ../../include/odp/api/abi-default/debug.h:26:53: note: expanded from
>>>>>>>>> macro
>>>>>>>>> 'ODP_STATIC_ASSERT'
>>>>>>>>> #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg)
>>>>>>>>> ^~~~
>>>>>>>>> odp_packet.c:55:20: note: cast that performs the conversions of a
>>>>>>>>> reinterpret_cast is not allowed in a constant expression
>>>>>>>>> ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet
>>>>>>>>> invalid not 0");
>>>>>>>>> ^
>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>> Since you want to breech the strong type you need to use the
>>>>>>>>>> internal `odp_typeval()` macro here.
>>>>>>>>>> ```
>>>>>>>>>> ODP_STATIC_ASSERT(_odp_typeval(ODP_EVENT_INVALID) == 0, "Event
>>>>>>>>>> invalid not 0");
>>>>>>>>>> ```
>>>>>>>>>> etc. I've verified that this works fine for clang, so no need for
>>>>>>>>>> the conditional compilation.
>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>> Should this be in its own commit? Different function.
https://github.com/Linaro/odp/pull/437#discussion_r165116521
updated_at 2018-01-31 16:54:11
