Support iwyu and clang-tidy

[tcojean]

Add support for clang-tidy 1 and iwyu 2 tools.

Overview

These tools allow to statically find potential programming issues and include
issues, respectively. They are able to find a lot of potential issues (both bugs
or style problems), so they are a useful addition for contributors. For now, no
specific configuration is included but it is possible to give a style file which
would control what we consider to be errors and what are not.

The use of the tools is controlled through the respective GINKGO_WITH_CLANG_TIDY
and GINKGO_WITH_IWYU variables. They are turned off by default, but their
availability is highlighted in the main README.md

Here is an example of what these tools bring in terms of analysis. Looking at what is outputted here, there are actual problems (such as not including memory header).

clang-tidy output for coo_kernels.cpp

/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:95:53: warning: parameter 'exec' is unused [misc-unused-parameters]
void spmv2(std::shared_ptr<const ReferenceExecutor> exec,
                                                    ^~~~~
                                                     /*exec*/
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:95:53: warning: the parameter 'exec' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:105:19: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
            c->at(coo_row[i], j) += coo_val[i] * b->at(coo_col[i], j);
                  ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:105:37: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
            c->at(coo_row[i], j) += coo_val[i] * b->at(coo_col[i], j);
                                    ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:105:56: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
            c->at(coo_row[i], j) += coo_val[i] * b->at(coo_col[i], j);
                                                       ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:114:62: warning: parameter 'exec' is unused [misc-unused-parameters]
void advanced_spmv2(std::shared_ptr<const ReferenceExecutor> exec,
                                                             ^~~~~
                                                              /*exec*/
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:114:62: warning: the parameter 'exec' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:127:19: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
            c->at(coo_row[i], j) +=
                  ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:128:29: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
                alpha_val * coo_val[i] * b->at(coo_col[i], j);
                            ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:128:48: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
                alpha_val * coo_val[i] * b->at(coo_col[i], j);
                                               ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:138:72: warning: parameter 'exec' is unused [misc-unused-parameters]
void convert_row_idxs_to_ptrs(std::shared_ptr<const ReferenceExecutor> exec,
                                                                       ^~~~~
                                                                        /*exec*/
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:138:72: warning: the parameter 'exec' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void convert_row_idxs_to_ptrs(std::shared_ptr<const ReferenceExecutor> exec,
                              ~~~                                      ^
                              const                                   &
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:167:64: warning: parameter 'exec' is unused [misc-unused-parameters]
void convert_to_dense(std::shared_ptr<const ReferenceExecutor> exec,
                                                               ^~~~~
                                                                /*exec*/
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:167:64: warning: the parameter 'exec' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:182:20: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
        result->at(coo_row[i], coo_col[i]) += coo_val[i];
                   ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:182:32: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
        result->at(coo_row[i], coo_col[i]) += coo_val[i];
                               ^
/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:182:47: warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]
        result->at(coo_row[i], coo_col[i]) += coo_val[i];

Include What You Use output with coo_kernels.cpp

Warning: include-what-you-use reported diagnostics:

/home/tcojean/Workspace/software/ginkgo-github/core/matrix/coo_kernels.hpp should add these lines:
#include <memory>                         // for shared_ptr
#include "ginkgo/core/base/executor.hpp"  // for DefaultExecutor

/home/tcojean/Workspace/software/ginkgo-github/core/matrix/coo_kernels.hpp should remove these lines:
- #include <ginkgo/core/base/types.hpp>  // lines 37-37
- #include <ginkgo/core/matrix/coo.hpp>  // lines 38-38
- #include <ginkgo/core/matrix/csr.hpp>  // lines 39-39

The full include-list for /home/tcojean/Workspace/software/ginkgo-github/core/matrix/coo_kernels.hpp:
#include <ginkgo/core/matrix/dense.hpp>   // for Dense, Coo, Csr
#include <memory>                         // for shared_ptr
#include "ginkgo/core/base/executor.hpp"  // for DefaultExecutor
---

/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp should add these lines:
#include "ginkgo/core/base/types.hpp"                  // for size_type, GKO...

/home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp should remove these lines:
- #include <ginkgo/core/base/exception_helpers.hpp>  // lines 36-36
- #include <ginkgo/core/base/math.hpp>  // lines 37-37
- #include <ginkgo/core/matrix/csr.hpp>  // lines 38-38

The full include-list for /home/tcojean/Workspace/software/ginkgo-github/reference/matrix/coo_kernels.cpp:
#include "core/matrix/coo_kernels.hpp"
#include <ginkgo/core/matrix/dense.hpp>                // for Dense, Coo, Csr
#include "ginkgo/core/base/types.hpp"                  // for size_type, GKO...
#include "reference/components/format_conversion.hpp"  // for convert_idxs_t...
---

[thoasm]

Very useful tools. However, when I enable both on my system, it stops compiling and I get the following error:

/home/thomas/projects/ginkgo_github_2/omp/matrix/coo_kernels.cpp:36:10: error: 'omp.h' file not found **[clang-diagnostic-error]**

It seems like it does not find the omp.h file, and then just cancels the compilation.
I did this with a complete new cmake and make.
Here is the cmake command I used:

cmake -G "Unix Makefiles" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
    -DCMAKE_BUILD_TYPE=Debug \
    -DGINKGO_DEVEL_TOOLS=ON -DGINKGO_BUILD_TESTS=ON \
    -DGINKGO_BUILD_REFERENCE=ON -DGINKGO_BUILD_OMP=ON -DGINKGO_BUILD_CUDA=ON -DGINKGO_CUDA_ARCHITECTURES="50" \
    -DBUILD_SHARED_LIBS=ON \
    -DGINKGO_WITH_CLANG_TIDY=ON \
    -DGINKGO_WITH_IWYU=ON -DGINKGO_IWYU_PATH="/home/thomas/software/include-what-you-use/build/bin/include-what-you-use" \
    ../..

Is there a way to prevent this issue? I think it would be nice if it would only add information as warnings, so it would still compile, even if there are many issues.

[thoasm]

I tested it without the iwyu, and it still crashed with the same error, so the clang-tidy does not work properly on my system. Is there a way to forward the OpenMP information to clang-tidy, or to tell clang-tidy to ignore this header file?

[thoasm]

Testing it with iwyu only works on my system, so it definitively looks like clang-tidy is the problem for me.
On a different topic: Is there a way for this to be run on the CI system? I think it would be very useful information when doing code review.

[tcojean]

The behavior you have with clang-tidy is weird, by default I believe everything is treated as a warning. I will try to see whether I can reproduce this and if there is a way around this.

[thoasm]

I have clang version clang 8.0.0-4 if that helps.

[tcojean]

Did you install the openmp package? Maybe the problem is that you do not have OpenMP support with clang? In my case, I do not get this error.

[thoasm]

Do I have to install an OpenMP package on Arch linux? I thought it always comes with the compiler.
I at least did not do that. Unfortunately, I cannot test that right now since it happened on my work laptop, which I did not bring with me. I am testing right now what happens on my Ubuntu.

[thoasm]

Apparently, there is an openmp package for Arch that I did not know about, which is an optional requirement for clang...
That might actually be the problem, since I am pretty sure I did not install it myself. It works fine on my Ubuntu.

The remaining question I have is: Should we make this a step in the CI system for easier code review, or would that be too much?

[tcojean]

I will add this to the CI system, I'm not sure how much it will help currently though as there is a lot of problems so it's hard to see if there is any new error or not.

[thoasm]

I guess we would definitively have to search for the file that was edited, but I think that we can fix these issues for the future PRs one after the other.

[thoasm]

The CI system apparently has a similar problem to my Arch Linux (does not find a particular header file):

/usr/lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/cxxabi_init_exception.h:38:10: fatal error: 'stddef.h' file not found

Taken from the CI output.

[tcojean]

The problem is actually with iwyu though, clang-tidy worked fine. That is surprising at first I thought it would be the opposite due to the clang-version being low.

[tcojean]

Looks like this is a known issue with iwyu. Depending how well the maintainers do the packaging, it may not properly find standard headers.
include-what-you-use/include-what-you-use#679

I'll try to get around this.

[pratikvn]

Even in the clang-tidy step there seem to be similar errors.

[tcojean]

You're both right. It did not fail for some reason so I thought all was well there.

[thoasm]

@pratikvn is correct. There are multiple 'stddef.h' file not found [clang-diagnostic-error] errors, however, they do not lead to a compile termination (probably since they are in system header instead of in the project headers).
Examples for the errors:

/usr/lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/cxxabi_init_exception.h:38:10: error: 'stddef.h' file not found [clang-diagnostic-error]
/usr/include/limits.h:123:16: error: 'limits.h' file not found [clang-diagnostic-error]
/usr/include/wchar.h:39:11: error: 'stdarg.h' file not found [clang-diagnostic-error]

Taken from this CI output

It seems to be limited to stddef.h, stdarg.h and limits.h.

[tcojean]

This is now fixed.

[thoasm]

LGTM!

[pratikvn]

LGTM!