[PATCH] Add PSTL internal namespace qualifications

Thomas Rodgers trodgers@redhat.com
Mon Apr 1 23:01:00 GMT 2019


>> We shouldn't include <cassert> in the std::lib, the uses of assert
>> should be changed to __glibcxx_assert instead (which is enabled by
>> defining _GLIBCXX_ASSERTIONS).
>>
>
> This has to come through one of the PSTL library configuration
> macros because the "right assert" upstream won't be __glibcxx_assert.

There are a number of locations in the upstream PSTL sources where
<cassert> is included and assert() is used. Additionally, the TBB
backend uses __TBB_ASSERT(). I'm going to follow up with a separate
patch that introduces __PSTL_ASSERT() and makes everything consistent,
with __PSTL_ASSERT expanding to __glibcxx_assert in libstdc++.

Tom.

Thomas Rodgers writes:

> Jonathan Wakely writes:
>
>> On 29/03/19 12:12 -0700, Thomas Rodgers wrote:
>>>Prevent ADL related weirdness.
>>>
>>>	* include/pstl/algorithm_impl.h: Add namespace qualification.
>>>	* include/pstl/execution_defs.h: Add namespace qualification.
>>>	* include/pstl/execution_impl.h: Add namespace qualification.
>>>	* include/pstl/numeric_impl.h: Add namespace qualification.
>>>	* include/pstl/parallel_backend_tbb.h: Add namespace qualification.
>>>	* include/pstl/unseq_backend_simd.h: Add namespace qualification.
>>>	* include/pstl/parallel_backend_utils.h: Include <cassert>.
>>
>> We shouldn't include <cassert> in the std::lib, the uses of assert
>> should be changed to __glibcxx_assert instead (which is enabled by
>> defining _GLIBCXX_ASSERTIONS).
>>
>
> This has to come through one of the PSTL library configuration
> macros because the "right assert" upstream won't be __glibcxx_assert.
>
>>>@@ -285,7 +286,7 @@ _ForwardIterator2
>>> __pattern_walk2_n(_ExecutionPolicy&&, _ForwardIterator1 __first1, _Size n, _ForwardIterator2 __first2, _Function f,
>>>                   _IsVector is_vector, /*parallel=*/std::false_type) noexcept
>>
>> I missed these before, but we have non-uglified 'n' and 'f' and
>> 'is_vector' here. Almost all uses of "is_vector" are inside a comment
>> like /*is_vector=*/ but here it's a real parameter name.
>>
>> In practice if users do something stupid like
>> #define n 123
>> #define f 456
>> then they won't be able to include these headers anyway, because
>> TBB uses those as parameter names (so users that are using these
>> headers with TBB can't defines such dumb macros). But when we get a
>> non-TBB backend that won't be the case, and we should be uglifying all
>> names declared in our own headers on principle.
>>
>>> {
>>>-    return __brick_walk2_n(__first1, n, __first2, f, is_vector);
>>>+    return __internal::__brick_walk2_n(__first1, n, __first2, f, is_vector);
>>> }
>>>
>>> template <class _ExecutionPolicy, class _RandomAccessIterator1, class _Size, class _RandomAccessIterator2,
>>>@@ -294,7 +295,7 @@ _RandomAccessIterator2
>>> __pattern_walk2_n(_ExecutionPolicy&& __exec, _RandomAccessIterator1 __first1, _Size n, _RandomAccessIterator2 __first2,
>>>                   _Function f, _IsVector is_vector, /*parallel=*/std::true_type)
>>
>> And 'n' and 'f' here.
>>
>>> {
>>>-    return __pattern_walk2(std::forward<_ExecutionPolicy>(__exec), __first1, __first1 + n, __first2, f, is_vector,
>>>+    return __internal::__pattern_walk2(std::forward<_ExecutionPolicy>(__exec), __first1, __first1 + n, __first2, f, is_vector,
>>>                            std::true_type());
>>> }
>>>
>>
>> We can fix the 'n' and 'f' and 'is_vector' names in another patch, for
>> this one please just remove <cassert> again and change assert(...) to
>> __glibcxx_assert(...).



More information about the Libstdc++ mailing list