Add cpplint to CI

[cbachhuber]

Addressing #378: add cpplint to the CI pipeline with a set of checks that require small changes (<10 lines) in existing code.

Currently, cpplint is expected to fail (solutions to these issues are already implemented, though not yet pushed). At this moment, this PR checks whether cpplint works in the azure pipeline and whether the return value of cpplint is used. Once that is verified, I'll push the code resolving the issues.

You can run cpplint on your machine by checking out this PR and executing

pip install cpplint
cpplint --counting=detailed --recursive examples include/CLI

Please review the checks that are so far excluded in file CPPLINT.cfg. We can discuss those checks in this or in future PRs.

Advice on integrating cpplint into CI is welcome, I'm an azure pipeline beginner. Hints on running the CI pipeline on my local machine are welcome as well :)

[codecov]

Codecov Report

Merging #400 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #400   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        3580   3580           
=====================================
  Hits         3580   3580

Impacted Files	Coverage Δ
include/CLI/Timer.hpp	100% <ø> (ø)	⬆️
include/CLI/Validators.hpp	100% <ø> (ø)	⬆️
include/CLI/App.hpp	100% <100%> (ø)	⬆️
include/CLI/TypeTools.hpp	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b3a6b6...0309fdb. Read the comment docs.

[cbachhuber]

Command cpplint --counting=detailed --recursive examples include/CLI does not seem to be called inside the pulled docker container: azure says that command cpplint is unknown.

How can I change the pipeline such that this command is called inside docker, where cpplint is available?

[henryiii]

Hint: Notice the first line of the job step:

/bin/bash --noprofile --norc /home/vsts/work/_temp/6092fe76-9723-4597-b2a3-114e50bc8d41.sh

Does not indicate it's running through docker. Scrolling up, there's no container starting lines in previous steps either. So that should be an indicator that the container is not being used. Now I'm not sure why Azure didn't complain about the pool mapping having an extra unused key...

[henryiii]

Here's Azure's container docs; it's quite hard to find, IMO, since "docker" brings up things like how to build a docker image, how to host one, etc, etc, etc. Not how to run on one. https://docs.microsoft.com/en-us/azure/devops/pipelines/process/container-phases?view=azure-devops

Like GitHub Actions (they are really reskinned versions of mostly the same thing), you can specify a container per-step as well.

[cbachhuber]

Hint: Notice the first line of the job step [...]

That did the trick, thanks a lot! I will also have a look at the link such that I can do fewer [WIP]-PRs in future ;)

I would like to ask your opinion on the flags that are to be discussed:

1. legal/copyright: requires a copyright message in the beginning of every file. This project is already covered by the LICENSE file, so this is not necessary imho.
2. runtime/references: would like to replace non-const references with pointers. Message example: include/CLI/Config.hpp:86: Is this a non-const reference? If so, make const or use a pointer: std::string &name [runtime/references] [2]. I personally like (non-const) references and don't see a threat in them, so I would not use this check.
3. runtime/int: Message example: include/CLI/TypeTools.hpp:598: Use int16/int64/etc, rather than the C type long [runtime/int] [4]. This flag makes sense, and int64_t works with std::stoll(). Only 4 changes required.
4. build/include_order: Message example: include/CLI/TypeTools.hpp:7: Found C++ system header after other header. Should be: TypeTools.h, c system, c++ system, other. [build/include_order] [4]. I think it would be nice to have ordered includes. 24 issues found.

I will follow your guidance on these flags. I can include them in this PR (increasing the diff) or create separate PRs.

[henryiii]

1. We probably should have a little header at the top of each with copyright info, pointing at LICENSE.
2. This would be a major change, completely changing the whole API. Needs to stay off.
3. This would add a new include, but isn't unreasonable.
4. We should fix the include order.

At least point 4 can go in, and maybe 3 and/or 1.

[phlptp]

I think I would recommend waiting on 1 and 3 until after a 1.9 release. Any new header would need to be handled well in the single file generator. I think it reasonable and probably desirable but there may be reasons to wait on it until after the single file generator is updated.

3 is also probably desirable but may involve changing some of the API, which might be more appropriate in the 2.0 release.

2 I am also not sure I agree with. I sort of understand why Google does it that way but the same rational doesn't apply to most other libraries. pointers come with nullability concerns so it would require a lot of modifications that I am not sure are useful in CLI11.

for 4 it is probably fine as long as it doesn't interfere with the clang-format since that has some header checks as well I believe. (at least is does on our other repos).

[cbachhuber]

Sounds very good! I'll do the following:

1. Wait until after 1.9 release
2. Put to unused checks
3. Looking at the issues reported by cpplint, this should be doable without an API change. If yes, I'm going to include it in this PR.
4. I'll try to include this check in this PR if there is no conflict with clang-format.

[phlptp]

The changes seem fine to me.

I should look up why cpplint does the headers in that order. I think I have always put the local headers first so they can't rely on the system headers. But maybe there is a good reason for the opposite.

[cbachhuber]

The changes seem fine to me.

I should look up why cpplint does the headers in that order. I think I have always put the local headers first so they can't rely on the system headers. But maybe there is a good reason for the opposite.

Thanks for your review! Regarding your comment: you're right, and Google style guidelines recommend it as you propose for the related header (i.e. a.hpp should be the first include of a.cpp). The headers that are shifted down in this PR are other project header files, not the related header file. The reasoning is that other project header files are themselves responsible for managing their includes in the same way, I think.

[jzakrzewski]

other project header files are themselves responsible for managing their includes in the same way

I'd say it encourages bad practise. With the "other project headers" after system headers it's easy to miss an include and make the header not self-contained (like you have to include vector before your project header or the thing will not compile).

[henryiii]

Putting the system includes before the other project headers does look odd and possibly error prone to me. Does Google explain why they recommend this? LLVM seems to do more explaining on the "why" of the recommendations.

[jzakrzewski]

I don't see official explanation from Google but there https://stackoverflow.com/a/54348720 is one possible explanation (maybe not why that order but at least why they could consider it safe). It doesn't really speak to me though.
There seems to be projects that explicitly deviate from the guidelines in that matter exactly to avoid unwanted dependencies: https://github.com/googleapis/google-cloud-cpp/blob/master/doc/cpp-style-guide.md#order-of-includes

[cbachhuber]

I'm also unable to find a proper explanation for this in Google's C++ style guide. Also, cpplint's check build/include_order cannot be modified. Therefore, I suggest removing this check from cpplint.

[henryiii]

This would add a new include, but isn't unreasonable.

Any file that uses the c std types needs <cstdint>. It is very likely any other std library header will include that one, but let's be pedantic and add it to the files that now use it. Otherwise, it looks good to go!

[cbachhuber]

Ah, thanks for reminding me of this. I read that one should use int64_t and companions in the std:: namespace. This is because headers that define parts of C, such as cstdint, are required to provide them in the std:: namespace, and may (not must!) in addition add them to the global namespace. The latter might change, however, so you're on the safe side when using these symbols as defined in the std:: namespace.

There are few more lines that need to be changed to follow this rule; I'll open a separate PR for fixing those after this PR has been merged. Ok?