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

Choose C++ style guideline #5

Closed
tigrannajaryan opened this issue Oct 2, 2019 · 7 comments
Closed

Choose C++ style guideline #5

tigrannajaryan opened this issue Oct 2, 2019 · 7 comments

Comments

@tigrannajaryan
Copy link
Member

C++ is a big language that can be used in many different ways. We need to choose a style and stick to it.

A couple popular starting points (can be chosen as is or as the starting point for our own style):

Other style suggestions are welcome.

@maxgolov
Copy link
Contributor

I think we should definitely embrace CppCoreGuidelines.

Re. coding style - we should add clang-format to the build. It's a popular tool accessible from most of the most common editors (vim, Visual Studio, vscode, eclipse), as well as we can use git cl format to reformat command-line, following about what the Chromium source code repo is doing:
https://chromium.googlesource.com/chromium/src/+/master/docs/clang_format.md

We should discuss what's the best fit - LLVM, Google, Chromium, or other styles. Maybe we should also allow for specific exporter dev to have their own preferred style (formatting-wise) via .clang-format file in their module sub-directory.

My personal preference would be something in the middle between Linux and LLVM formatting style. Others may have a different opinion. We should use automated process (e.g. clang-format) to enforce whatever we agree upon..

@g-easy
Copy link
Contributor

g-easy commented Oct 22, 2019

I agree re: automatic formatting and enforcing it.

@maxgolov
Copy link
Contributor

maxgolov commented Oct 25, 2019

I wrote some quick script for adding a mini version of .clang-format'ter without needing Python (Windows, Linux, Mac), i.e. lightweight wrapper for git cl format , a bit less sophisticated than Chromium clang-format command. I can send a PR with the script, plus 4-5 coding style examples, as well as how-to doc with links to IDE plugins for auto-formatting using that file. We can review the styles, agree on what seems reasonable to majority, and check-in the .clang-format file in workspace root.

@rnburn
Copy link
Contributor

rnburn commented Oct 28, 2019

Since the core cpp guidelines don't have a full naming convention, which one should we adopt?

My preference would be to take one of the standard conventions (or at least start there) so that we don't have to go through debating how to name each component in the language.

Two that we might consider:

  • Google's naming convention
  • Envoy's naming convention -- similar to Google but with some differences in capitalization (e.g. function names are doFoo() instead of DoFoo())

Anyone have a preference? or other convention they want to consider?

@g-easy
Copy link
Contributor

g-easy commented Oct 28, 2019

I prefer the Google one.

@g-easy
Copy link
Contributor

g-easy commented Oct 28, 2019

#5 (comment) Regarding autoformat, OpenCensus used to have a relatively simple script: tools/format.sh

It runs clang-format + buildifier + cmake-format. The user can then review and commit the results. Importantly, it also runs during CI.

@meastp
Copy link
Contributor

meastp commented Nov 4, 2019

#5 (comment) Regarding autoformat, OpenCensus used to have a relatively simple script: tools/format.sh

It runs clang-format + buildifier + cmake-format. The user can then review and commit the results. Importantly, it also runs during CI.

If you do this, please write it in a language that also supports Windows.

I would prefer a CI verification step that created/added a commit if something was missing (according to the clang-format, buildifer and cmake-format) because users that do not care about cmake don't necessarily have it installed and similarly for bazel (and thus can not run the script locally).

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

6 participants