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

Decide on and enforce house C++ style #994

Open
Baltoli opened this issue Feb 23, 2024 · 2 comments
Open

Decide on and enforce house C++ style #994

Baltoli opened this issue Feb 23, 2024 · 2 comments

Comments

@Baltoli
Copy link
Contributor

Baltoli commented Feb 23, 2024

Introduction

After #992 1, I think it's worth putting the effort in to properly clean up our C++ code with a house style. We use a bunch of different styles in the C++ code, and there is no strong consensus for what the correct solution will look like. This issue is an attempt at an opinionated set of defaults that we can bikeshed, then start to apply gradually to the code.2

Note

There are a few things that are out of scope here. We already have a consistent structural style for the code that's enforced conveniently by clang-format, and similarly we have a lot of semantic best practices and conventions being enforced by clang-tidy. We can therefore restrict the bikeshedding here to naming aesthetics and consistency.

Proposed Style

#define MACRO_IF_NEEDED(x) x

namespace some_namespace_name {

enum class some_enum {
  variant_a,
  variant_b,
};

template <typename TypeParameter>
class some_class_name {
public:
  void member_function() const;

private:
  int member_variable_ = 0;
};

void some_class_name::member_function() const {
  auto local_var = f();
  free_function(member_variable_);
}

void free_function(int my_parameter_name) {
  ...
}

}

Some things we should also consider that are not directly naming style:

Migration Strategy

Once we agree on a style, it should be easily achievable to migrate one stylistic element at a time using clang-tidy.

Footnotes

  1. We'd have avoided this pain by enforcing a consistent naming prefix for our C code so as to remain hygienic when interfacing with the outside world. Nobody other than us is naming their functions kllvm_arena_alloc!

  2. We can hopefully clean up or improve some of the bigger, gnarlier functions that are currently exempted from clang-tidy's congitive-complexity warnings when we do this.

@ehildenb
Copy link
Member

All for it! I think it's best to just pick a style, make the PR, and then let people nit at it. But I think general consensus is that a consistent style is more important than any particular choice.

@Baltoli
Copy link
Contributor Author

Baltoli commented Feb 23, 2024

Indeed - it's blocked on #995 but once that's resolved I can use the tooling to just apply this style globally and see if there are nitpicks

@Baltoli Baltoli mentioned this issue Mar 4, 2024
rv-jenkins pushed a commit that referenced this issue Mar 5, 2024
This PR is a mechanical[^1] fix for #994; it selects some default naming
conventions for C++ code and applies them across the project. Open to
bikeshedding if there are particular places anyone doesn't like, but as
with all these changes the most important thing is having the tool
enabled at all.

The changes might not be perfect; there's a lot of surface area to cover
here but they should be pretty good. I've tested against the K
integration test suite to make sure nothing is broken there.

[^1]: `clang-tidy` + a pile of one-off `sed` scripts to cover edge
cases; fortunately style is easier to enforce than to apply so
`clang-tidy` will do on its own moving forwards.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

## Summary by CodeRabbit

- **Refactor**
- Standardized naming conventions across the codebase for improved
consistency and readability. This includes converting camelCase to
snake_case and adjusting enum values to PascalCase where applicable.
- Renamed various entities (functions, variables, classes) to adhere to
the new naming convention, enhancing code clarity.
- **Style**
- Updated comments and documentation to reflect changes in naming
conventions and code structure.
- **Chores**
- Adjusted method signatures and variable names for consistency with the
newly adopted naming standards.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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

2 participants