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

Refactor the code according to the Google C++ Style Guide #27

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Yida-Lin
Copy link
Member

#25

Standardize code style according to https://google.github.io/styleguide/cppguide.html.

Didn't have time to do functionality change, only style cleanup for now.

Note that some method name changes require the code changes in SigViewer as well.

@Yida-Lin Yida-Lin requested review from tstenner and cbrnr May 17, 2020 09:40
@Yida-Lin Yida-Lin self-assigned this May 17, 2020
@Yida-Lin Yida-Lin changed the title 25: Refactor the code according to the Google C++ Style Guide Issue-25: Refactor the code according to the Google C++ Style Guide May 17, 2020
@Yida-Lin Yida-Lin changed the title Issue-25: Refactor the code according to the Google C++ Style Guide 25: Refactor the code according to the Google C++ Style Guide May 17, 2020
@Yida-Lin Yida-Lin changed the title 25: Refactor the code according to the Google C++ Style Guide Issue-25: Refactor the code according to the Google C++ Style Guide May 17, 2020
@cbrnr
Copy link
Collaborator

cbrnr commented May 17, 2020

Where does the style guide say that function names must be camel case?

@Yida-Lin
Copy link
Member Author

Yida-Lin commented May 17, 2020

@cbrnr I agree it looks a bit odd; it's not even camel case, it's Pascal case. It says it here: https://google.github.io/styleguide/cppguide.html#Function_Names

The C++ standard library does not adopt this style though http://www.cplusplus.com/reference/algorithm/

@Yida-Lin Yida-Lin marked this pull request as draft May 17, 2020 10:47
@cbrnr
Copy link
Collaborator

cbrnr commented May 17, 2020

It's your decision. Coming mainly from Python this looks really weird.

@Yida-Lin
Copy link
Member Author

@cbrnr By looking at this thread, looks like the snake_case() makes the most sense to me.

@cbrnr
Copy link
Collaborator

cbrnr commented May 17, 2020

Yes, I'd go with the conventions used in the standard lib (at least for this case).

@Yida-Lin Yida-Lin marked this pull request as ready for review May 17, 2020 21:51
@Yida-Lin
Copy link
Member Author

@cbrnr @tstenner think I cleaned up most stylistic things; most files changes in this PR are documentation changes auto generated by Doxygen, which you can ignore. Only xdf.h, xdf.cpp, and README.md are relevant.

@tstenner
Copy link
Contributor

Personally, I prefer the LLVM over the Google style guide, but any style is an improvement over an inconsistent or undocumented style.

Two things: Having the compiled doxygen documentation in the repository adds a lot of noise to the commits. For liblsl, we use readthedocs to host the doxygen output so only the doxygen configuration files are in the repository. One other option is to include the doxygen output in the released archives so anyone downloading it has the html documentation.

The other things I'm missing is a clang-format file (labstreaminglayer example, interactive clang-format generator so that Visual Studio / QtCreator can automatically reformat the code according to the style guide.

@cbrnr cbrnr changed the title Issue-25: Refactor the code according to the Google C++ Style Guide Refactor the code according to the Google C++ Style Guide May 18, 2020
@Yida-Lin
Copy link
Member Author

Yida-Lin commented May 21, 2020

@tstenner Thank you for your input, sounds great. I will look into these over the weekend.

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

Successfully merging this pull request may close these issues.

3 participants