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

[FR] Coding style checker and workflow integration #21

Closed
fujitatomoya opened this issue May 30, 2023 · 6 comments · Fixed by #22
Closed

[FR] Coding style checker and workflow integration #21

fujitatomoya opened this issue May 30, 2023 · 6 comments · Fixed by #22

Comments

@fujitatomoya
Copy link
Collaborator

Feature request

Motivation

follow up of #20.
coding style needs to be checked by every single PR and commits to make sure incoming PRs are compatible with current development policy.

Feature description

We can rely on cpplint, and integrate it to github workflow.

@Windrow14
Copy link
Collaborator

Windrow14 commented Jun 14, 2023

Commands to be added in the pipeline:

pip install cpplint
find -iname *.h | xargs cpplint
find -iname *.c | xargs cpplint
find -iname *.hpp | xargs cpplint
find -iname *.cpp | xargs cpplint

However, cpplint is just a static code checker. It does not modify code automatically by itself. We need to modify code according to its advice manually.


cpplint --recursive --extensions=hpp,cpp,h,c ./

This one is better.

@Windrow14
Copy link
Collaborator

Windrow14 commented Jun 14, 2023

I have a problem.
Cpplint complains about Lines should be <= 80 characters long [whitespace/line_length] [2]. I can resolve it by cutting the line in the middle, but there seems no standard for where to cut the line and how many white space we should add at the front of the new line or where to align with.

For example:

### Line too long:
        if (SRT_INVALID_SOCK != srt_connect(srtsock, (const struct sockaddr *)&addr, sa_len)) {
### solution 1
        if (SRT_INVALID_SOCK != srt_connect(srtsock,
                                (const struct sockaddr *)&addr, sa_len)) {
### solution 2
        if (SRT_INVALID_SOCK != srt_connect(srtsock,
                                            (const struct sockaddr *)&addr, sa_len)) {
### solution 3
        if (SRT_INVALID_SOCK !=
            srt_connect(srtsock, (const struct sockaddr *)&addr, sa_len)) {

...

There are hundreds of solutions can make cpplint silent, but most of them looks even ugly than before...

@Windrow14
Copy link
Collaborator

I have a problem. Cpplint complains about Lines should be <= 80 characters long [whitespace/line_length] [2]. I can resolve it by cutting the line in the middle, but there seems no standard for where to cut the line and how many white space we should add at the front of the new line or where to align with.

For example:

### Line too long:
        if (SRT_INVALID_SOCK != srt_connect(srtsock, (const struct sockaddr *)&addr, sa_len)) {
### solution 1
        if (SRT_INVALID_SOCK != srt_connect(srtsock,
                                (const struct sockaddr *)&addr, sa_len)) {
### solution 2
        if (SRT_INVALID_SOCK != srt_connect(srtsock,
                                            (const struct sockaddr *)&addr, sa_len)) {
### solution 3
        if (SRT_INVALID_SOCK !=
            srt_connect(srtsock, (const struct sockaddr *)&addr, sa_len)) {

...

There are hundreds of solutions can make cpplint silent, but most of them looks even ugly than before...

There is a standard about function calling and arguments here: https://google.github.io/styleguide/cppguide.html#Function_Calls

@Windrow14
Copy link
Collaborator

I'd like to neglect some warnings from cpplint, such as: Do not use namespace using-directives. Use using-declarations instead. [build/namespaces] [5], <mutex> is an unapproved C++11 header. [build/c++11] [5], and some warnings related toSRT_DATA_MAP_INITIALIZER

@fujitatomoya
Copy link
Collaborator Author

However, cpplint is just a static code checker. It does not modify code automatically by itself. We need to modify code according to its advice manually.

I think that is developer's responsibility. Until workflow with cpplint green light, we will not merge the PR.

There are hundreds of solutions can make cpplint silent, but most of them looks even ugly than before...

if it does not work with cpplint, we can use // NOLINT with justification.

@fujitatomoya
Copy link
Collaborator Author

closing in favor of #22

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