Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VOTE: const west or east const #40

Open
j-stephan opened this issue Sep 22, 2020 · 25 comments
Open

VOTE: const west or east const #40

j-stephan opened this issue Sep 22, 2020 · 25 comments

Comments

@j-stephan
Copy link
Collaborator

j-stephan commented Sep 22, 2020

Currently there is no way to enforce a ruling on this since both clang-format and clang-tidy don't offer any related options. AFAIK there is a PR to clang-format in the pipeline but who knows when that will be incorporated. In any case we should decide on this as well so that we don't need to vote again once the option arrives.

The arguments for const west are found in the C++ Core Guidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rl-const

tl;dr: It is more familiar to programmers and already present in a lot of code bases.

const west examples:

const int some_var = 1;
const int* some_ptr;
const int* const constant_ptr;
const char* a_function();
const std::string& someClass::a_method();

The arguments for east const are presented here: https://mariusbancila.ro/blog/2018/11/23/join-the-east-const-revolution/

tl;dr: It is more logical and consistent.

east const examples:

int const some_var = 1;
int const* some_ptr;
int const* const constant_ptr;
char const* a_function();
std::string const& someClass::a_method();

VOTE

const west east const
0 1
@j-stephan
Copy link
Collaborator Author

@psychocoderHPC Can you notify the PIConGPU developers and the alpaka developers? Apparently I can't do this myself, maybe I'm missing some permissions.

@PrometheusPi
Copy link
Member

const west east const
1 1

@bernhardmgruber
Copy link
Collaborator

const west east const
2 1

The argument that east const is more logical because then the const keyword always applies to the entity left to it is sound. However, I increasingly rarely find situations where there could be an ambiguity. The examples in Jan's article proposing east const are mostly related to the use of pointers. But with better abstractions in recent C++ versions (references, string_view, span, unique_ptr, etc.) the need for pointers diminishes.

On the contrary, the information that a variable is const is of prime importance to me. It is the first thing I want to know about a variable. Thus I prefer to have const as the first keyword.

@sbastrakov
Copy link
Member

const west east const
3 1

(even despite one of my old Reddit accounts was literally EastConst)

@ax3l
Copy link
Member

ax3l commented Sep 24, 2020

const west east const
3 2

east const, the only consistent const.

Especially useful when passing pointers of vectors of unique ptrs of maps of something and trying to find out which parts are mutable.

west const randomness:

void const_west(
    const std::vector<
        std::unique_ptr<
            std::map< std::string, const float * >
        >
    > * const x
) {}

vs. "what's left is const": 🧑‍🍳

void east_const(
    std::vector<
        std::unique_ptr<
            std::map< std::string, float const * >
        >
    > const * const x
) {}

@theZiz
Copy link
Member

theZiz commented Sep 24, 2020

const west east const
3 3

@ax3l
Copy link
Member

ax3l commented Sep 24, 2020

Can you notify the PIConGPU developers and the alpaka developers?

@ComputationalRadiationPhysics/picongpu-developers @ComputationalRadiationPhysics/alpaka-developers

@KseniaBastrakova
Copy link

const west east const
4 3

@BenjaminW3
Copy link
Member

const west east const
4 4

@michaelsippel
Copy link
Member

const west east const
4 5

@theZiz
Copy link
Member

theZiz commented Sep 24, 2020

🍿

@sbastrakov
Copy link
Member

sbastrakov commented Sep 24, 2020

So as expected east style vs. west is a hot topic.

@j-stephan
Copy link
Collaborator Author

Do we have any rules for a tie?

@steindev
Copy link
Member

Do we have any rules for a tie?

Invite an odd number of people and let them vote...

@SimeonEhrig
Copy link
Contributor

If we have tie, I will choose ;-P
At the moment I'm not sure. Normally I use the west style, but the east style makes it easier to understand the code. But with big class names it looks stupid ...

@theZiz
Copy link
Member

theZiz commented Sep 24, 2020

Invite an odd number of people and let them vote...

This is an... odd idea! :trollface: SCNR 😁

@bertwesarg
Copy link
Member

const west east const
4 6

but feel free to ignore me

@bernhardmgruber
Copy link
Collaborator

bernhardmgruber commented Sep 24, 2020

const west east const
4 6
but feel free to ignore me

Unfortunately it does not work that way :/ Your vote counts just as much as the people who really care. If east/west const is not of importance to you, then please do not vote. Please :)

@bernhardmgruber
Copy link
Collaborator

void east_const(
    std::vector<
        std::unique_ptr<
            std::map< std::string, float const * >
        >
    > const * const x
) {}

I really dislike when people bring up such examples. You would very seldomly pass a pointer to std::vector. And you would also seldomly mark value parameters const. Thus, if we do not write void f(int const i) then we should not write void f(int* const i).

Generally, we should pass pointers very rarely, because there is a manifold of better alternatives, like references, smart pointers, pass by value, std::span . And with pointers gone, there is not much space for confusing where the const belongs to.

@bertwesarg
Copy link
Member

Unfortunately it does not work that way :/ Your vote counts just as much as the people who really care. If east/west const is not of importance to you, then please do not vote. Please :)

Then please take my vote seriously ;-)

@ax3l
Copy link
Member

ax3l commented Sep 24, 2020

Generally, we should pass pointers very rarely, because there is a manifold of better alternatives, like references, smart pointers, pass by value, std::span . And with pointers gone, there is not much space for confusing where the const belongs to.

Yeah I am not making that up, for example I see plenty of cases per day where people have a host-container of pointers to device objects and quickly unwrap the pointer to pass it to a device lambda in-place. Smart pointers don't help here and it's a quick way to get things passed from host to device in lambda-heavy code, so I don't judge them.

We are talking C++14 code bases as of today, C++17 transitions potentially this or early next year, etc. (re: C++20 std::span and actually need for C++2b std::mdspan for common host-device patterns... and then cuda/hip/sycl compilers and libs need to support them on the host-device interface, too.)

@bernhardmgruber
Copy link
Collaborator

Yeah I am not making that up, for example I see plenty of cases per day where people have a host-container of pointers to device objects and quickly unwrap the pointer to pass it to a device lambda in-place. Smart pointers don't help here and it's a quick way to get things passed from host to device in lambda-heavy code, so I don't judge them.

Interesting pattern. But this probably stems from CUDA trading device memory regions in pointers which I find questionable. It potentially misleads users into believing they can actually perform operations on a T* as they would with ones not returned from a device memory allocation function. I guess that might be a reason why many other APIs chose to wrap these device memory regions into distinct buffer objects, thus leveraging the type system communicate more clearly.

We are talking C++14 code bases as of today, C++17 maybe with the next CUDA release, etc. (re: C++20 std::span and actually need for C++2b std::mdspan for common host-device patterns... and then cuda/hip/sycl compilers and libs need to support them on the host-device interface, too.)

I think I have to disagree here. The coding guidelines are aiming to set a standard for a wide variety of projects within CRP and teams as CASUS. We cannot assume any C++ version here. That is subject to each project. LLAMA for example is on C++17 and has an optional concept (with is C++20).

As for std::span, if you do not have it yet, there are great library implementations available with support for CUDA etc. I also do not have std::format for now so I use libfmt until the switch to C++20. With std::mdspan, there is the Kokkos reference implementation, ready to use today: https://github.com/kokkos/mdspan.

@ax3l
Copy link
Member

ax3l commented Sep 24, 2020 via email

@psychocoderHPC
Copy link
Member

const west east const
4 7

I do not really like east const for constness but I do not see that we will get rid of pointers for heterogeneous apps/libs where we need to pass pointer to the device. Smart pointer will not help with that and I do not believe that we will get smart-pointer soon (<5years) for GPU programming.

@j-stephan
Copy link
Collaborator Author

Voting closed, the winner is east const.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests