oneapi-src / oneDPL

oneAPI DPC++ Library (oneDPL) https://software.intel.com/content/www/us/en/develop/tools/oneapi/components/dpc-library.html
Apache License 2.0
720 stars 114 forks source link

Move the use of ranges-v3 to "standalone" DR #1761

Closed akukanov closed 1 month ago

akukanov commented 1 month ago

@BenBrock @lslusarczyk

In this patch I propose to support ranges-v3 in a different manner.

The "common" DR code in oneDPL would support redefinition of the stdrng namespace alias, as well as the _ONEDPL_DR_STD_RANGES_NAMESPACE macro (the patch also removes one extra leading underscore for that macro, to fit oneDPL naming rules).

Providing alternative definitions for ranges-v3 would be done in the "standalone" DR repository, like below:

// File: dr/detail/ranges_v3_shim.hpp
#pragma once

// In order to use ranges-v3 instead of C++20 ranges,
// #define _ONEDPL_DR_STD_RANGES_SHIM_HEADER <dr/detail/ranges_v3_shim.hpp>
// before including oneDPL DR headers

#include <range/v3/all.hpp>

namespace stdrng = ::ranges;

#define _ONEDPL_DR_STD_RANGES_NAMESPACE ranges

The macro _ONEDPL_DR_STD_RANGES_SHIM_HEADER should name the "replacement" shim header, in this case, the one that redirects to ranges-v3. It should be defined before including the "common" part from oneDPL, so that the custom shim is included by the main shim header in oneDPL.

BenBrock commented 1 month ago

I think this is a pretty good idea, although I do think we have some testing to do to make sure that everything still works with range-v3. @lslusarczyk, what do you think?

lslusarczyk commented 1 month ago

As I understand the goal/advantage of the change is to

What about

#ifndef _ONEDPL_DR_STD_RANGES_NAMESPACE
// by default use ranges from stdlib
#    include <ranges>
#    define _ONEDPL_DR_STD_RANGES_NAMESPACE std::ranges
#endif
namespace stdrng = ::_ONEDPL_DR_STD_RANGES_NAMESPACE

then code of user who wants ranges-v3 can be

#include <range/v3/all.hpp>
#define _ONEDPL_DR_STD_RANGES_NAMESPACE ranges
// include onedpl headers

Looks simpler to me. In particular there is no need for _ONEDPL_DR_STD_RANGES_SHIM_HEADER to be defined by user. @akukanov , what do you think?

akukanov commented 1 month ago

@lslusarczyk It's definitely simpler; I would be fine with this if you think it works good enough in the anticipated use cases.

The approach with a special macro has a potential benefit of this macro being defined in the build system outside of the C++ code base, thus guaranteed to be in place when oneDPL headers are included. Additionally, if there is a need to handle some API difference between std::ranges and range-v3, the extra shim header could in theory do it.

Perhaps if all range-v3 support can be put in some header in the standalone DR, which is then pre-included one way or the other (maybe also by a compiler option) into all necessary translation units, there is not much practical difference for users.