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

Establish C++ coding guidelines #45195

Open
stephanosio opened this issue Apr 28, 2022 · 10 comments
Open

Establish C++ coding guidelines #45195

stephanosio opened this issue Apr 28, 2022 · 10 comments
Assignees
Labels
area: C++ area: Coding Guidelines Coding guidelines and style RFC Request For Comments: want input from the community

Comments

@stephanosio
Copy link
Member

Introduction

Establish C++ coding guidelines to be used in the Zephyr repository, and implement means to enforce them.

Problem description

While we have multiple coding and style guideline checkers such as checkpatch.pl and guideline_check.py for the C language, we do not for the C++ language. It is necessary to establish and enforce repository-wide C++ coding and style guidelines in order to ensure code quality and consistency.

Proposed change

  1. Adopt one or more C++ coding and style guidelines for use in the Zephyr repository.
    • Refer to the "Guideline Candidates" below.
  2. Adopt tools to enforce such guidelines in the CI.
  3. Document the guidelines.

Guideline Candidates

(please feel free to edit this RFC and add any other guidelines that you think would be good for consideration)

Enforcement Tool Candidates

  • clang-tidy, enforces C++ Core Guidelines (and more)
  • Cpplint, enforces Google C++ Style Guide
@stephanosio stephanosio added RFC Request For Comments: want input from the community area: C++ labels Apr 28, 2022
@stephanosio stephanosio self-assigned this Apr 28, 2022
@stephanosio
Copy link
Member Author

@stephanosio stephanosio added area: Coding Guidelines Coding guidelines and style dev-review To be discussed in dev-review meeting labels Apr 28, 2022
@yperess
Copy link
Collaborator

yperess commented May 13, 2022

Hey, just wanted to post here and let you know I haven't forgotten about this. I'm looking at the options and will post my thoughts about this as soon as I have formed a strong opinion.

@stephanosio stephanosio mentioned this issue May 19, 2022
8 tasks
@chrta
Copy link
Collaborator

chrta commented May 23, 2022

I think it might be worth to consider the styles supported by clang-tidy and check if one of them can be used, see https://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-style-options (below BasedOnStyle)
There are also links to style guides from other projects.

@yperess
Copy link
Collaborator

yperess commented May 24, 2022

OK, so a likely unpopular opinion. I've uploaded a .clang-format file here with a rough guideline. It's non standard in C/C++ but it's close to the style guide for Kotlin. The driving factor here is to minimize diffs and merge conflicts.
clang-format.txt

@stephanosio
Copy link
Member Author

@yperess Can you provide some examples/sample code in that style? It is hard to see what it looks like with just the .clang-format file.

@yperess
Copy link
Collaborator

yperess commented May 24, 2022

@yperess Can you provide some examples/sample code in that style? It is hard to see what it looks like with just the .clang-format file.

Sure, here are some examples, it mostly revolves around having things on separate lines. It takes up more vertical space, but makes reviews easier and results in fewer merge conflicts.

int action(
  int color,
  char alpha,
  float
);

int fooNS::FooClass::action(
    int color,
    char alpha,
    float
) {
  return doSomething(
      color,
      alpha
  );
}

ptr
  ->getSelf()
  ->getSelf()
  ->getSelf()
  ->getSelf();

@stephanosio
Copy link
Member Author

FYI @cfriedt

@cfriedt
Copy link
Member

cfriedt commented Jun 14, 2022

I added some comments in #41307, but IMHO, it makes sense to consolidate on clang-format because it supports both C and C++.

@cfriedt
Copy link
Member

cfriedt commented Jun 14, 2022

In terms of linter (clang-tidy), it's possible to run it only on changes too
https://clang.llvm.org/extra/doxygen/clang-tidy-diff_8py_source.html
@stephanosio

It might also be good to mention clang-format in the description, as it's more of a code formatter and clang-tidy is more of a linter. Of course, scan-build for static analysis, etc.

@stephanosio stephanosio removed the dev-review To be discussed in dev-review meeting label Jun 15, 2022
@aborisovich
Copy link
Collaborator

C++ Core Guidelines, maintained by the Standard C++ Foundation certainly has my vote. Those guys really though it through.
In addition to those guidelines, please consider following convention of file namings (I'm not sure how it may be enforced except core review remarks):

  • header files containing C++ code incompatible with C (eg. namespaces, class definitions) should have ".hpp" extension
  • header files containing mixed C/C++ code should have currently used ".h" extension

This way it would be easy to find files that should not be included by C code. Just an idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C++ area: Coding Guidelines Coding guidelines and style RFC Request For Comments: want input from the community
Projects
Status: No status
Development

No branches or pull requests

5 participants