onnxruntime
onnxruntime copied to clipboard
microsoft
Create clang-tidy lint
https://github.com/microsoft/onnxruntime/actions/runs/3092428102/jobs/5003682677
looking at the build output, it appears that there are no files checked. perhaps the filter should be adjusted?
Run ZedThree/[email protected]
...
Diff from GitHub PR:
[<PatchedFile: .clang-tidy>, <PatchedFile: .github/workflows/lint.yml>]
include: *.[ch], file list now: []
include: *.[ch]xx, file list now: []
include: *.[ch]pp, file list now: []
include: *.[ch]++, file list now: []
include: *.cc, file list now: []
include: *.hh, file list now: []
exclude: , file list now: []
No files to check!
@edgchen1 I think that's because the lint is only checking diffs!
ah ok, that makes sense.
as a side note, we'll probably need to generate a compile_commands.json file to configure clang-tidy properly.
as a side note, we'll probably need to generate a compile_commands.json file to configure clang-tidy properly.
Absolutely. I haven't figured out how to do that though. @snnn any suggestions?
as a side note, we'll probably need to generate a compile_commands.json file to configure clang-tidy properly.
Absolutely. I haven't figured out how to do that though. @snnn any suggestions?
may be able to refer to this as a starting point: https://github.com/microsoft/onnxruntime/blob/40749124b132a8a114a639548eec76abe6152e57/tools/ci_build/github/azure-pipelines/mac-objc-static-analysis-ci-pipeline.yml#L20-L29
a few notes:
--build_objcis probably unnecessary. and--use_coremlenabling the CoreML EP may be as well. that pipeline is specifically analyzing the Objective-C code.- it is not ideal as it does a full build of ORT. I think that was done to generate the ONNX protobuf source files. it certainly could be done more efficiently, perhaps by building the corresponding cmake target directly.
also, not sure which EPs we should enable for this. I suppose we can try with only the CPU EP first.
Use clangd as C++ language server gives you a uniform editing experience span C/C++ and CUDA and HIP code. clang-tidy and clang-format is also integrated. All you need is the compile_commands.json.
Current command
python tools/ci_build/build.py --build_dir build --update --cmake_extra_defines CMAKE_EXPORT_COMPILE_COMMANDS=ON
Strange: https://github.com/microsoft/onnxruntime/actions/runs/3099926945/jobs/5019640574#step:8:39
clang-tidy failed with return code 134 and error: LLVM ERROR: Cannot chdir into "/home/runner/work/onnxruntime/onnxruntime/build/Debug"!
Strange: https://github.com/microsoft/onnxruntime/actions/runs/3099926945/jobs/5019640574#step:8:39
clang-tidy failed with return code 134 and error: LLVM ERROR: Cannot chdir into "/home/runner/work/onnxruntime/onnxruntime/build/Debug"!
wonder if setting base_dir is needed since compile_commands.json is generated outside of the action: https://github.com/ZedThree/clang-tidy-review#use-in-a-non-default-location
@edgchen1 @cloudhan should
modernize-use-nodiscard be disabled? Otherwise this is ready to merge.
@edgchen1 @cloudhan should
modernize-use-nodiscard be disabled? Otherwise this is ready to merge.
I can see the merits of using [[nodiscard]] more. However, we don't use it much in our existing code. @pranavsharma what do you think?
@edgchen1 @cloudhan should modernize-use-nodiscard be disabled? Otherwise this is ready to merge.
I can see the merits of using [[nodiscard]] more. However, we don't use it much in our existing code. @pranavsharma what do you think?
I think it'll be good to enable this. I've seen before that we ignored Status returns from some functions.
nondiscard is great. Better to keep it on and fix them from the code side.
@justinchuby Just found a new common annoying case:
Floating point literal has suffix 'f', which is not uppercase (fix available)clang-tidy(readability-uppercase-literal-suffix)
I think this should also be disabled.