-
Notifications
You must be signed in to change notification settings - Fork 890
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
libcudf needs a code style guide #479
Comments
Curious about opinions from: @mt-jones @harrism @eyalroz @williamBlazing @BradReesWork |
We also need to discuss whether each sub-module a Class, Name space, or just individual functions (current mode) |
Here is another popular styling guide https://github.com/isocpp/CppCoreGuidelines |
Indeed, I would be equally happy using the ISO CPP guidelines. It has the same benefit of already being supported by |
So, several things:
Finally, here are a couple of relevant videos about guidelines and conventions:
|
@eyalroz thanks for the detailed feedback.
A few key points come to mind:
A possible way forward that balances both community input and speed of delivery is to appoint 2-3 motivated individuals from across the active contributors to work together to lay the foundation of the libcudf style guide. Once those individuals have come up with the 80% solution, we can open it up for community input. |
No no, I'm absolutely against that. If there is something specific in there that you find useful, I would consider it, but certainly not defaulting to accepting that style guide. Just as an example: You would not have been able to write the type_dispatcher using their style guide, because it says (in its current version): "Code should not use C++14 or C++17 features". And this illustrates how the goals of that style guide are incompatible with ours. Now, I'm not saying I reject all rules in there. Some I would adopt, some I would be neutral about, some I would reject. But again, not a default-adopt on that thing. Also, I believe, but am not certain, that the Google style guide is at least to some extent in disagreement with the C++ Core Guidelines. Don't take my word for it though, I'd need to do more reading to check.
I'm not at all sure this would be the case. Are you saying this because of the lack of C++14 in CUDA 9.x? Or the partial usability of the standard library? I think we could manage not to have to accommodate much, if at all.
We start from the here and now, which is code of varying quality and little consistency, and no rules. I disagree that the first step should be adopting a complete rule-set. Here's how I imagine this proceeding:
|
I am in favor of a minimal style guide that grows as it meets the needs of our specific team. As @BradReesWork mentions, there's a need to identify how sub modules should be scoped. Instead of adopting an entire style guide, perhaps we could start with a GH issue addressing that specific challenge? The goal of a style guide is to reduce development time and cost. Exactly how much cost can be saved I think is super complicated. The cost of adopting Google's C++ guide and enforcing it is high - regular debates over whether or not the style is matched, time spent studying it, efforts put into backporting our existing code to match the style, and more. @eyalroz's list above also has a fairly high cost - 7 steps that will take considerable time across the team to negotiate and finally settle down on. Style guides can also have a surprisingly amount of social cost as developers passionately argue. I prefer a slow approach to this challenge. :) |
@thomcom : The thing is, you want a bit of an involved process before preventing people from programming a certain way... but if we wanted something faster, we could have a shorter process for suggested rules with no explicit objections. Something like:
|
Here is the draft guide I wrote for cuIO. It's meant to be easily digestible and to highlight the most common issues. For more detailed rules and justifications, the Google style guide is available. General structure
Use of language features
Best practices
Naming/formatting
For everything that is not explicitly specified here, follow the Google C++ style guide. (https://google.github.io/styleguide/cppguide.html). |
A lot/most of these items are fine by me, but:
(I'm not commenting item-by-item here of course.) Also, @vuule : Are you aware of the C++ Core Guidlines project? |
Yes. Luckily, it looks like we are now goto-free. If the consensus is that some of these rules are irrelevant, we can remove them. These are based on the code I've seen, and that is still a small part of cuDF. As you pint out, the list is incomplete (e.g. doing work in constructors). We can always add more exception to the list. The current list is what I came up with in a short period, and a round of reviews would certainly improve it a lot. I made the list to contain exceptions from GSG, rather than a full list of rules, because I expect that there would be (at least) an order of magnitude fewer exception than rules. Most exceptions would be related to exceptions, as you mentioned. I am aware of the Core guidelines. I believe some of my items are inspired by those rules. I did not take them too much into account, as they are still incomplete and not that widely adopted. Also, I found that some of the rules there are hard to adhere to without using their https://github.com/Microsoft/GSL. |
@vuule : I wasn't suggesting we use the C++ Core Guidelines as a style guide, just as an inspiration. I don't accept your premise: You're assuming we've silently accepted the GSG as a basis for libcudf's style, possibly with exceptions. But - we haven't. If you believe we need to accept the GSG, you need to argue for it as a whole and convince us we should accept it as a default. and I would be against that for three reasons:
|
I have written this guide primarily for cuIO, and we have a clear consensus that GSG is a good base style for us. If stakeholders of the libcudf as a whole are against using GSG, my proposal is not relevant in this discussion. |
@vuule, you're suggesting that a consensus would be needed to reject GSG as a basis for a style guide. It is actually the opposite - a style guide would require a consensus, or overwhelming majority support, to adopt. I'm sorry for being somewhat argumentative here, but I'm quite upset with what's happening here, process-wise. |
Closing this. #5273 is more relevant today. |
Feature request
In order to provide a high quality and consistent code base, there should be a code style guide created and enforced.
It is non-trivial to create a quality code style guide from scratch, therefore I suggest we should use the Google C++ style guide as a starting point and modify as necessary (i.e., move from C++11 to C++14 and add rules for CUDA code).
https://google.github.io/styleguide/cppguide.html
The benefit of the Google C++ style guide is that is already well established and used throughout industry and other open source projects.
Furthermore, there is already widespread linter support for ensuring conformance with the style guide, e.g., cpplint and clang-tidy.
Likewise, the ISO CPP Core Guidelines would be an equally good starting point for a libcudf code style guide: https://github.com/isocpp/CppCoreGuidelines
It also has the advantage of being natively supported by clang-tidy.
The text was updated successfully, but these errors were encountered: