Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Add ability to place Thrust in a custom namespace. #1464

Merged
merged 1 commit into from
Jul 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
5 changes: 5 additions & 0 deletions cmake/ThrustHeaderTesting.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ foreach(thrust_target IN LISTS THRUST_TARGETS)
set(headertest_target ${config_prefix}.headers)
add_library(${headertest_target} OBJECT ${headertest_srcs})
target_link_libraries(${headertest_target} PUBLIC ${thrust_target})
# Wrap Thrust/CUB in a custom namespace to check proper use of ns macros:
target_compile_definitions(${headertest_target} PRIVATE
"THRUST_WRAPPED_NAMESPACE=wrapped_thrust"
"CUB_WRAPPED_NAMESPACE=wrapped_cub"
)
thrust_clone_target_properties(${headertest_target} ${thrust_target})

# Disable macro checks on TBB; the TBB atomic implementation uses `I` and
Expand Down
2 changes: 1 addition & 1 deletion dependencies/cub
Submodule cub updated 104 files
9 changes: 9 additions & 0 deletions testing/cmake/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,12 @@ if (THRUST_CPP_FOUND AND THRUST_CUDA_FOUND)
${extra_cmake_flags}
)
endif()

# Check that namespace macros are used correctly:
add_test(
NAME thrust.test.cmake.check_namespace
COMMAND
"${CMAKE_COMMAND}"
-D "Thrust_SOURCE_DIR=${Thrust_SOURCE_DIR}"
-P "${CMAKE_CURRENT_LIST_DIR}/check_namespace.cmake"
)
93 changes: 93 additions & 0 deletions testing/cmake/check_namespace.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
# Check all files in thrust to make sure that they use
# THRUST_NAMESPACE_BEGIN/END instead of bare `namespace thrust {}` declarations.
#
# This is run as a ctest test named `thrust.test.cmake.check_namespace`, or
# manually with:
# cmake -D "Thrust_SOURCE_DIR=<thrust project root>" -P check_namespace.cmake

cmake_minimum_required(VERSION 3.15)

set(exclusions
# This defines the macros and must have bare namespace declarations:
thrust/detail/config/namespace.h
)

function(count_substrings input search_regex output_var)
string(REGEX MATCHALL "${search_regex}" matches "${input}")
list(LENGTH matches num_matches)
set(${output_var} ${num_matches} PARENT_SCOPE)
endfunction()

set(bare_ns_regex "namespace[ \n\r\t]+thrust[ \n\r\t]*\\{")

# Validation check for the above regex:
count_substrings([=[
namespace thrust{
namespace thrust {
namespace thrust {
namespace thrust {
namespace thrust
{
namespace
thrust
{
]=]
${bare_ns_regex} valid_count)
if (NOT valid_count EQUAL 6)
message(FATAL_ERROR "Validation of bare namespace regex failed: "
"Matched ${valid_count} times, expected 6.")
endif()

set(found_errors 0)
file(GLOB_RECURSE thrust_srcs
RELATIVE "${Thrust_SOURCE_DIR}"
"${Thrust_SOURCE_DIR}/*.h"
"${Thrust_SOURCE_DIR}/*.inl"
"${Thrust_SOURCE_DIR}/*.cu"
)

foreach(src ${thrust_srcs})
if (${src} IN_LIST exclusions)
continue()
endif()

file(READ "${Thrust_SOURCE_DIR}/${src}" src_contents)

count_substrings("${src_contents}" "${bare_ns_regex}" bare_ns_count)
count_substrings("${src_contents}" THRUST_NS_PREFIX prefix_count)
count_substrings("${src_contents}" THRUST_NS_POSTFIX postfix_count)
count_substrings("${src_contents}" THRUST_NAMESPACE_BEGIN begin_count)
count_substrings("${src_contents}" THRUST_NAMESPACE_END end_count)
count_substrings("${src_contents}" "#include <thrust/detail/config.h>" header_count)

if (NOT bare_ns_count EQUAL 0)
message("'${src}' contains 'namespace thrust {...}'. Replace with THRUST_NAMESPACE macros.")
set(found_errors 1)
endif()

if (NOT prefix_count EQUAL 0)
message("'${src}' contains 'THRUST_NS_PREFIX'. Replace with THRUST_NAMESPACE macros.")
set(found_errors 1)
endif()

if (NOT postfix_count EQUAL 0)
message("'${src}' contains 'THRUST_NS_POSTFIX'. Replace with THRUST_NAMESPACE macros.")
set(found_errors 1)
endif()

if (NOT begin_count EQUAL end_count)
message("'${src}' namespace macros are unbalanced:")
message(" - THRUST_NAMESPACE_BEGIN occurs ${begin_count} times.")
message(" - THRUST_NAMESPACE_END occurs ${end_count} times.")
set(found_errors 1)
endif()

if (begin_count GREATER 0 AND header_count EQUAL 0)
message("'${src}' uses Thrust namespace macros, but does not (directly) `#include <thrust/detail/config.h>`.")
set(found_errors 1)
endif()
endforeach()

if (NOT found_errors EQUAL 0)
message(FATAL_ERROR "Errors detected.")
endif()
8 changes: 4 additions & 4 deletions testing/copy.cu
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <algorithm>
#include <list>
#include <iterator>
#include <thrust/detail/config.h>
#include <thrust/sequence.h>
#include <thrust/iterator/zip_iterator.h>
#include <thrust/iterator/counting_iterator.h>
Expand Down Expand Up @@ -733,15 +734,14 @@ struct only_set_when_expected_it
}
};

namespace thrust
{
THRUST_NAMESPACE_BEGIN
namespace detail
{
// We need this type to pass as a non-const ref for unary_transform_functor
// to compile:
template <>
struct is_non_const_reference<only_set_when_expected_it> : thrust::true_type {};
}
} // end namespace detail

template<>
struct iterator_traits<only_set_when_expected_it>
Expand All @@ -750,7 +750,7 @@ struct iterator_traits<only_set_when_expected_it>
typedef only_set_when_expected_it reference;
typedef thrust::random_access_device_iterator_tag iterator_category;
};
}
THRUST_NAMESPACE_END

void TestCopyWithBigIndexesHelper(int magnitude)
{
Expand Down
8 changes: 6 additions & 2 deletions testing/mr_disjoint_pool.cu
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ struct alloc_id
}
};

namespace thrust { namespace detail {
THRUST_NAMESPACE_BEGIN
namespace detail {
template<>
struct pointer_traits<alloc_id>
{
Expand All @@ -48,7 +49,10 @@ struct pointer_traits<alloc_id>
return reinterpret_cast<void *>(id.alignment);
}
};
}}

} // end namespace detail

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any style recommendations for the number of new lines between namespace enclosing? For now it seems to be different in each case:

rm /tmp/{2,3}
find . -name "*.h" -exec bash -c 'lines=$(rg --multiline -e "}\s*THRUST" {} | wc -l); echo 1 >> /tmp/$lines' \;
wc -l /tmp/{2,3}
rm /tmp/{2,3}
 4 /tmp/2
41 /tmp/3
find . -name "*.h" -exec bash -c 'lines=$(rg --multiline -e "} // namespace detail\s*THRUST" {} | wc -l); echo 1 >> /tmp/$lines' \;
wc -l /tmp/{2,3}
 1 /tmp/2
12 /tmp/3

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing code is all over the place with these sorts of style issues, so there's not a firm rule.

I generally prefer a single blank line. This is what our proposed .clang-format file will do, and we'll fix this when we get around to setting that up since we can automate it.

THRUST_NAMESPACE_END

class dummy_resource final : public thrust::mr::memory_resource<alloc_id>
{
Expand Down
43 changes: 43 additions & 0 deletions testing/namespace_wrapped.cu
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
// Wrap thrust and cub in different enclosing namespaces
// (In practice, you probably want these to be the same, in which case just
// set THRUST_CUB_WRAPPED_NAMESPACE to set both).
#define THRUST_WRAPPED_NAMESPACE wrap_thrust
#define CUB_WRAPPED_NAMESPACE wrap_cub

#include <thrust/device_vector.h>
#include <thrust/host_vector.h>
#include <thrust/iterator/constant_iterator.h>
#include <thrust/iterator/counting_iterator.h>
#include <thrust/transform.h>

#include <unittest/unittest.h>

// Test that we can use a few common utilities and algorithms from a wrapped
// namespace at runtime. More extensive testing is performed by the header
// tests and the check_namespace.cmake test.
void TestWrappedNamespace()
{
const std::size_t n = 2048;

const auto in_1_begin =
::wrap_thrust::thrust::make_constant_iterator<int>(12);
const auto in_2_begin =
::wrap_thrust::thrust::make_counting_iterator<int>(1024);

// Check that the qualifier resolves properly:
THRUST_NS_QUALIFIER::device_vector<int> d_out(n);

::wrap_thrust::thrust::transform(in_1_begin,
in_1_begin + n,
in_2_begin,
d_out.begin(),
::wrap_thrust::thrust::plus<>{});

::wrap_thrust::thrust::host_vector<int> h_out(d_out);

for (std::size_t i = 0; i < n; ++i)
{
ASSERT_EQUAL(h_out[i], static_cast<int>(i) + 1024 + 12);
}
}
DECLARE_UNITTEST(TestWrappedNamespace);
8 changes: 5 additions & 3 deletions testing/scan.cu
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#include <unittest/unittest.h>

#include <thrust/detail/config.h>

#include <thrust/scan.h>
#include <thrust/functional.h>
#include <thrust/iterator/discard_iterator.h>
Expand Down Expand Up @@ -583,15 +586,14 @@ struct only_set_when_expected_it
}
};

namespace thrust
{
THRUST_NAMESPACE_BEGIN
template<>
struct iterator_traits<only_set_when_expected_it>
{
typedef long long value_type;
typedef only_set_when_expected_it reference;
};
}
THRUST_NAMESPACE_END

void TestInclusiveScanWithBigIndexesHelper(int magnitude)
{
Expand Down
Loading