Add clang tidy check to CI

[coder3101]

Clang Tidy is a great static analysis tool for checking the quality of the code and to check if a code follows some standards like BOOST Standards.

This issue talks about adding to Github Actions CI clang-tidy checker. After this, the CI will report the clang-tidy results in a PR and in push events.

This will vastly ease up the review process in PR as the reviewer only have to care the logical correctness of the code instead of the standard conference with standards.

[coder3101]

@bassoy need your suggestion here.

In PR, clang-tidy can upload a clang-tidy-report.txt file as a comment. So people can view it and check what clang-tidy has reported. In push what should be the behaviour to report those clang-tidy warnings? Should CI fail if clang-tidy reports some warning or error? Should clang-tidy check be extended to test/tensor as well or example/tensor and include/boost/numeric/ublas/tensor is enough?

[bassoy]

In PR, clang-tidy can upload a clang-tidy-report.txt file as a comment. So people can view it and check what clang-tidy has reported.

Very good.

[coder3101]

Very good.

I have even better approach, Instead of uploading the textfile. What if CI can review the pull request with checks?

Something like this:

It will take some time as, currently I am writing the automation scripts in python.

[bassoy]

In push what should be the behaviour to report those clang-tidy warnings? Should CI fail if clang-tidy reports some warning or error?

In the first instance, I would like to put any output of clang-analyzer-* as an error. Also I we should include clang memory sanitizer and clang address sanitizer.

[bassoy]

Very good.

I have even better approach, Instead of uploading the textfile. What if CI can review the pull request with checks?

looks good to me.

[bassoy]

Very good.

I have even better approach, Instead of uploading the textfile. What if CI can review the pull request with checks?

looks good to me.

some checks can be also automatically fixed.

[coder3101]

The Review part by CI to PR is done, the check upon push will also be completed next day.

[coder3101]

some checks can be also automatically fixed.

Yes, however that should be done manually by the author, relying on code changes to CI for anything other than formatting could become a source of unknown bugs! clang-tidy smartly reports but we can fix them or ignore them. Moreover, the fix is suggested in the review by the CI, the PR proposer should fix them manually.

See the auto review system in #10

[bassoy]

some checks can be also automatically fixed.

Yes, however that should be done manually by the author, relying on code changes to CI for anything other than formatting could become a source of unknown bugs! clang-tidy smartly reports but we can fix them or ignore them. Moreover, the fix is suggested in the review by the CI, the PR proposer should fix them manually.

See the auto review system in #10

I am okay with your proposal.

[bassoy]

In push what should be the behaviour to report those clang-tidy warnings? Should CI fail if clang-tidy reports some warning or error?

In the first instance, I would like to put any output of clang-analyzer-* as an error. Also I we should include clang memory sanitizer and clang address sanitizer.

[coder3101]

In push what should be the behaviour to report those clang-tidy warnings? Should CI fail if clang-tidy reports some warning or error?

In the first instance, I would like to put any output of clang-analyzer-* as an error. Also I we should include clang memory sanitizer and clang address sanitizer.

These sanitizers can be enabled and disabled with clang++ test CI. I will look into them, we just need to pass the argument to clang compiler while compiling and running unit tests. -fsanitizer=address, memory

[bassoy]

In push what should be the behaviour to report those clang-tidy warnings? Should CI fail if clang-tidy reports some warning or error?

In the first instance, I would like to put any output of clang-analyzer-* as an error. Also I we should include clang memory sanitizer and clang address sanitizer.

These sanitizers can be enabled and disabled with clang++ test CI. I will look into them, we just need to pass the argument to clang compiler while compiling and running unit tests. -fsanitizer=address, memory

[coder3101]

It can have a separate issue of its own!

[bassoy]

https://github.com/BoostGSoC20/ublas/blob/f7841f622bbec57415bce6789137111ac7a42279/.ci/clang-tidy-review.py#L5

We need to discuss about licensing because we would need to relicense the file and state which portion (what did you modify) falls under Boost license

[coder3101]

We need to discuss about licensing because we would need to relicense the file and state which portion (what did you modify) falls under Boost license.

I don't know how to do relicense but I would be happy to learn it. Tell me procedure, I will follow them!

There are around 10 lines that I have changed, which I can easily tell you after git diff.

[bassoy]

I don't know how to do relicense but I would be happy to learn it. Tell me procedure, I will follow them! There are around 10 lines that I have changed, which I can easily tell you after git diff.

Actually, it is not too difficult. You just need to mark which lines you added and under which license it falls which is the Boost license in our case.

However, the question is if we want to have a software which carries a license other than Boost. Although really nice, I would like you to hold that python script back as we need to discuss this with Stefan or David.

[coder3101]

Although really nice, I would like you to hold that python script back as we need to discuss this with Stefan or David.

I will instead wait on the author to allow me to Relicense this. Check the request here.

If Peter does not allows or is not willing to relicense it. I will write a script myself, moreover his script had some issues with lookup functions which was fixed by me after I learnt the GitHub API and unidiff. So at this point, I myself can write a script for us under BSL but that will take some time and let's see.

[bassoy]

Although really nice, I would like you to hold that python script back as we need to discuss this with Stefan or David.

I will instead wait on the author to allow me to Relicense this. Check the request here.

If Peter does not allows or is not willing to relicense it. I will write a script myself, moreover his script had some issues with lookup functions which was fixed by me after I learnt the GitHub API and unidiff. So at this point, I myself can write a script for us under BSL but that will take some time and let's see.

Peter already put his code under the MIT license. It would make no sense if he removes it and puts the Boost license. You can simply use his code but his license must stay. MIT license allows us many things, but not to remove the license form.

In this case, I would like to put this issue on hold and discuss that. There is no need to rush here. Please see in gitter my suggestion regarding next steps.

[coder3101]

Clang-tidy check for Push to the repository has been set. Upon every push to the repository, the this job which can be configured by .clang-tidy in the root of the project is run.

The warnings are shown in Job log, you can modify the .clang-tidy to choose which warnings to be treated as an error and which of them as warnings.

[coder3101]

I am working on writing my own script to review PR, which I will register as BSL. Afterwards, this issue will be tagged as done and I will proceed to prepare things for the PR to boostorg/ublas

EDIT: It is too late, 1 AM now, I will write the script tomorrow!

[coder3101]

I think it's useless to even write my own script as it has some pip dependency which are not under BSL. I will proceed in this direction of only Cem allows!

[bassoy]

I think it's useless to even write my own script as it has some pip dependency which are not under BSL. I will proceed in this direction of only Cem allows!

Boost.ublas does not offer python with pip installed libraries as part of our library. So the simple usage of python and pip is okay, as it is for gcc, clang, etc. however, things are different if we integrate an already licensed file into our software/repo.

Usage of free licensed software is okay, integration and modification is more complicated.

[bassoy]

Let us postpone this step. You can create an extra issue and stash your change regarding your script - so we have the to pick it up fast and integrate it.

[coder3101]

You also need to choose which of the clang-tidy tests you want to run and which of them you want to treat as an error, so CI will fail on error and warnings will be ignored. After you have chosen a sensible .clang-format file. I suggest you choose a .clang-tidy file as well.

[coder3101]

modernize-use-trailing-return-type checks for clang-tidy should be excluded. It just Spams the report for every function. I also see that you do not treat any warning as error. The CI will never fail in that case.

[bassoy]

I agree with modernize-use-trailing-return-type.

You also need to choose which of the clang-tidy tests you want to run and which of them you want to treat as an error, so CI will fail on error and warnings will be ignored. After you have chosen a sensible .clang-format file. I suggest you choose a .clang-tidy file as well.

It was on purpose. I do not know all the checks. I will look at the outputs and determine the necessity over time. The suggestion modernize-use-trailing-return-type generates too much noise, I agree.

[bassoy]

There are some checks like

• cppcoreguidelines-avoid-magic-numbers
• readability-magic-numbers

which should be excluded only for the tensor examples in ublas/examples/tensor/

[coder3101]

It takes checks to run as regex. So we can use regex that matches everything modern-* except modern-use-trailing-return-type.

This answer shows how to write a regex that does this.

[bassoy]

Rephrased my https://github.com/BoostGSoC20/ublas/issues/4#issuecomment-643657757

[coder3101]

Rephrased my #4 (comment)

You can put a .clang-tidy file in that directory and clang-tidy will use that file. This works for clang-format and it should work for clang-tidy too, although I haven't checked it.

[bassoy]

Kindly try out your proposal for solving https://github.com/BoostGSoC20/ublas/issues/4#issuecomment-643657757

[coder3101]

which should not be excluded only for the tensor examples in ublas/examples/tensor/

Did you mean

which should be excluded only for the tensor examples in ublas/examples/tensor/

[coder3101]

I find it more logical that examples should exclude magic number tests because in examples we create many tensors with differents extents to show use case. In side the library however magic numbers are important.

I am doing this:

• Disable magic number for examples/tensor
• Enable magic numbers for other/directories

Its done, 108 Errors are now coming up! instead of 200 that used to come.

[bassoy]

which should not be excluded only for the tensor examples in ublas/examples/tensor/

Did you mean

which should be excluded only for the tensor examples in ublas/examples/tensor/

my mistake. yes you are right.

[bassoy]

Its done, 108 Errors are now coming up! instead of 200 that used to come.

very good. thanks.

[bassoy]

how difficult is it to still enable your proposed github action until we have a (perhaps self-maintained) solution?

[coder3101]

how difficult is it to still enable your proposed github action until we have a (perhaps self-maintained) solution?

Did you mean the proposed auto review script?

[bassoy]

yes

[coder3101]

how difficult is it to still enable your proposed GitHub action until we have a (perhaps self-maintained) solution?

how difficult is it to still enable your proposed github action until we have a (perhaps self-maintained) solution?

It is very easy, I still have the auto review scripts and workflow file in another branch clang-tidy-review-check. I just need to merge or cherry-pick changes from that branch.

I was looking at Zach Laine's new proposed Boost.Text library review process and some people pointed that a file in his library was not licensed to BSL, He said to the reviewers that he has taken permission from original author to relicense the code to BSL. In our case also the original author has granted the permission. We only need to wait for David or Stefan to review, Why re-invent the wheel? Even if a write a script myself. It will take some 1-2 days time.

[bassoy]

It is very easy, I still have the auto review scripts and workflow file in another branch clang-tidy-review-check. I just need to merge or cherry-pick changes from that branch.

Let us do the pull-request without review-script on clang-tidy first and then just merge it afterwards if everyone likes it.

He said to the reviewers that he has taken permission from original author to relicense the code to BSL.

Great.

Why re-invent the wheel?

Agree. Let's stay pragmatic here and first try to solve the auto report with the predefined github action and otherwise try to relicense code if necessary.

[bassoy]

After creating new issue, I will close this one.