Refactor clang-tidy

[cbachhuber]

This pull request

• Includes all applicable clang-tidy google coding convention flags (more google guidelines will be checked in future PRs with cpplint)
• Treats warnings from clang-tidy flag violations as errors, such that CI fails on clang-tidy violations, even if there is no auto-fix available
• Resolves the issues that are now treated as errors

Note: this PR does by far not cover all clang-tidy flags, it is just a first reasonable set.

[codecov]

Codecov Report

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

@@          Coverage Diff          @@
##           master   #389   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        3565   3565           
=====================================
  Hits         3565   3565

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

---

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 7a85aa9...cdd497d. Read the comment docs.

[henryiii]

Fix the two currently found clang-tidy errors in Config.hpp, such that we pass all CI tests

I'd love to have this in 1.9. Do you have this part ready, or at least know that the errors are?

[cbachhuber]

Sure, clang-tidy even offers auto-fixes for these rather simple warnings (which are treated as errors in this PR). Here is the relevant clang-tidy output:

CLI11/include/CLI/Config.hpp:115:8: error: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors]
    if(output.size() > 0 && output.back().name == "--") {
       ^~~~~~~~~~~~~~~~~
       !output.empty()

CLI11/include/CLI/Config.hpp:282:48: error: the 'empty' method should be used to check for emptiness instead of comparing to an empty object [readability-container-size-empty,-warnings-as-errors]
                    if(!(group == "Options" && opt->get_group() == "")) {
                                               ^~~~~~~~~~~~~~~~~~~~~~~
                                               opt->get_group().empty()

Should I open a separate PR for these small fixes? That would be a 5min job which I could easily do. What is the deadline for 1.9?

[henryiii]

The library is designed around the opposite of google-runtime-references, so that one can't be added. google-explicit-constructor is a good idea so I'll add the fixes from that (the .clang-format change to check it however will come from this PR). The int one adds an extra header for no reason, so is not a good idea either. The final one, the performance one, will have to be carefully added as the auto-add breaks the code.

[henryiii]

Please rebase, now the CI checks clang-tidy again (but currently only reports an error when a fix can be automatically applied - this PR should fix that).

[cbachhuber]

Thank you very much for #390! Good points about the flags, I'll follow your advice. I'll rebase and check whether CI reports errors if fixes can not be applied automatically. I'll also update the PR description once I'm clear about the remaining scope.

[cbachhuber]

CI failed, test successful. I'm going to remove flag cppcoreguidelines-owning-memory, because the CLI11 code reported by that flag is a 1:1 copy from canonical C++ example code.

[cbachhuber]

For flag google-readability-braces-around-statements, I chose 3 lines as upper limit, as this allows an

if()
    //Statement
else
    //Statement

construct without braces. Longer constructs require braces, see diff. Agreed?

I did not yet include google-build-using-namespace yet. This complains only about

/home/chris/dev/CLI11/include/CLI/StringTools.hpp:31:1: error: do not use namespace using-directives; use using-declarations instead [google-build-using-namespace,-warnings-as-errors]
using namespace enums;
^

I think this is a justified complaint. Should I include this check as well?

[henryiii]

The google-readability-braces-around-statements change looks great.

I think we can make this a using statement for the one << operator, then? If so, sure.

[cbachhuber]

Exactly, implemented the using-declaration. This PR is now ready for review.

[henryiii]

Thanks!