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:
We can have `_odp_pktio_index()` which does not perform such checks and
`odp_pktio_index()` which does `ODP_PKTIO_INVALID` check.
> Dmitry Eremin-Solenikov(lumag) wrote:
> 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_r165116904
updated_at 2018-01-31 16:55:25
