Justinchu/clang tidy test

[justinchuby]

Do not merge.

Test clang-tidy CI

[justinchuby]

@snnn With this test PR I am seeing a few checks that are particularly noisy / may not make sense (onnxruntime::Tensor* tensor = nullptr = value.GetMutable<onnxruntime::Tensor>();)

I am still learning c++ so I don't have good judgement on the suggestions generated. Do you have a recommended rules list we can modify based on the currect .clang-tidy file?

[snnn]

No, I don't. I will look into it this weekend.

[justinchuby]

@snnn With this test PR I am seeing a few checks that are particularly noisy / may not make sense (onnxruntime::Tensor* tensor = nullptr = value.GetMutable<onnxruntime::Tensor>();)

I am still learning c++ so I don't have good judgement on the suggestions generated. Do you have a recommended rules list we can modify based on the current .clang-tidy file?

@edgchen1 do you have any suggestions on .clang-tidy rules?

[edgchen1]

@snnn With this test PR I am seeing a few checks that are particularly noisy / may not make sense (onnxruntime::Tensor* tensor = nullptr = value.GetMutable<onnxruntime::Tensor>();) I am still learning c++ so I don't have good judgement on the suggestions generated. Do you have a recommended rules list we can modify based on the current .clang-tidy file?

@edgchen1 do you have any suggestions on .clang-tidy rules?

I don't have much at the moment - would need to familiarize myself with them more.

From a quick look at the list, "google" ones seem good since we follow their style guide. Some of the "bugprone", "cppcoreguidlines", "modernize", "performance", and "readability" ones look useful too, though perhaps we should pick and choose from them. For example, I think we can do without modernize-use-trailing-return-type, it's just stylistic.

Looking at the warnings in this PR, it appears that maybe clang-tidy isn't configured correctly. For example, a lot of the warnings from cppcoreguidelines-init-variables are actually referring to initialized variables. It also complains about being unable to find include files. Is the compile_commands.json file getting generated and passed to clang-tidy (-p option)?

[justinchuby]

Is the compile_commands.json file getting generated and passed to clang-tidy (-p option)?

No since I wasn't sure what the best way is to generate it. Is there a command I can run to create it?

[edgchen1]

Is the compile_commands.json file getting generated and passed to clang-tidy (-p option)?

No since I wasn't sure what the best way is to generate it. Is there a command I can run to create it?

That's a good question. We do have various build options, which ones should we test with? @snnn To generate it you can define CMAKE_EXPORT_COMPILE_COMMANDS when calling CMake (e.g., build.py --cmake_extra_defines CMAKE_EXPORT_COMPILE_COMMANDS=ON).

[edgchen1]

FYI, these warnings seem incorrect: https://github.com/microsoft/onnxruntime/pull/12655#discussion_r979134421 https://github.com/microsoft/onnxruntime/pull/12655#discussion_r979134422 https://github.com/microsoft/onnxruntime/pull/12655#discussion_r979134423 https://github.com/microsoft/onnxruntime/pull/12655#discussion_r979134424 https://github.com/microsoft/onnxruntime/pull/12655#discussion_r979134426

[justinchuby]

@edgchen1 that's a little more noise than what I hope it would produce! Should I turn off

misc-unused-parameters
readability-convert-member-functions-to-static
cppcoreguidelines-init-variables

for now? I am not sure why cppcoreguidelines-init-variables went wrong in particular

[edgchen1]

@edgchen1 that's a little more noise than what I hope it would produce! Should I turn off

misc-unused-parameters
readability-convert-member-functions-to-static
cppcoreguidelines-init-variables

for now? I am not sure why cppcoreguidelines-init-variables went wrong in particular

I think those are reasonable checks and it's ok to leave them on.

there are some errors from clang-tidy:

Output was:
error: unknown warning option '-Wno-nonnull-compare' [clang-diagnostic-unknown-warning-option]
/github/workspace/cmake/external/onnx/onnx/onnx_pb.h:51:10: error: 'onnx/onnx-ml.pb.h' file not found [clang-diagnostic-error]

https://github.com/microsoft/onnxruntime/actions/runs/3116529857/jobs/5054446243

which may be why we get these spurious warnings.

onnx/onnx-ml.pb.h is a generated file. should be able to generate it like this:

cmake --build build/Debug --config Debug --target onnx_proto

after the call to build.py:

https://github.com/microsoft/onnxruntime/blob/a2fa01b2426c62bbaf9e2e6d1fd6a4118a40fb0e/.github/workflows/lint.yml#L86-L92

[justinchuby]

@edgchen1 looking good now (to me)