Create clang-tidy CI #12653

justinchuby · 2022-08-19T16:21:01Z

Update clang-tidy config to prepare for creating a CI workflow to run clang-tidy.
Added clangtidy check in CI

cloudhan · 2022-09-20T03:58:22Z

.clang-tidy

@@ -1,6 +1,6 @@
 ---
 # turn off readability-braces-around-statements to allow single line statement like 'if (x == y) doSomething();'
-Checks:          '-*,cppcoreguidelines-*,google-*,readability-*,modernize-*,-readability-braces-around-statements,-google-runtime-references,-cppcoreguidelines-pro-type-reinterpret-cast'
+Checks:          '-*,cppcoreguidelines-*,google-*,readability-*,modernize-*,bugprone-*,-readability-braces-around-statements,-google-runtime-references,-cppcoreguidelines-pro-type-reinterpret-cast'


.clang-tidy file is a yaml file literally, if desired, you can actually make it mulit-line by using a yaml multiple line string, and clang-tidy itself happens to tolerate the \n tokens.

Done. Thanks for the suggestion. Do you have any thoughts on the kind of checks we should enable? Discussion happened in #12655 but was inconclusive.

@snnn @edgchen1

FYI, I've passed this question along to the core team.

@justinchuby I don't have suggestion to enable more checks, instead, I'd suggest to disable more.

modernize-concat-nested-namespaces, becaure we don't use it.

cppcoreguidelines-macro-usage, there are a lot of false-positives like

Function-like macro 'Foo' used; consider a 'constexpr' template function

readability-identifier-length, clang-tidy is bullshitting here, there are a lot of numerical code rely on short names to imporve readability.

google-readability-todo, Missing username/bug in TODO, which I am not sure we are practicing.

cppcoreguidelines-pro-bounds-array-to-pointer-decay Do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead. But our macros like ORT_RETURN_IF and LOGS are relying on it. We shall fix it, either way.

hint, onnxruntime/core/session/inference_session.cc is full of warning in current setting xDDD

@cloudhan I added all except for cppcoreguidelines-pro-bounds-array-to-pointer-decay, because it does seem desired from my understanding of your explanation, even though some of our macros have this behavior?

Removed -google-readability-braces-around-statements because google-readability-braces-around-statements.ShortStatementLines is set to 1.

Yeah, that is OK to keep cppcoreguidelines-pro-bounds-array-to-pointer-decay, it has its purpose. But I think someone should fix it from source code level. And if we add the magic comment to disable it at the macro definition side, I am not sure if it has effect on macro application site.

edgchen1 · 2022-09-20T21:41:11Z

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!

justinchuby · 2022-09-20T21:45:24Z

@edgchen1 I think that's because the lint is only checking diffs!

edgchen1 · 2022-09-20T21:56:01Z

@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.

justinchuby · 2022-09-20T21:56:58Z

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?

edgchen1 · 2022-09-21T00:45:00Z

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:

onnxruntime/tools/ci_build/github/azure-pipelines/mac-objc-static-analysis-ci-pipeline.yml

Lines 20 to 29 in 4074912

    
             - script: | 
        
                 python tools/ci_build/build.py \ 
        
                   --build_dir "$(Build.BinariesDirectory)" \ 
        
                   --cmake_generator "Ninja" \ 
        
                   --config Debug \ 
        
                   --build_shared_lib --use_coreml --build_objc \ 
        
                   --cmake_extra_defines CMAKE_EXPORT_COMPILE_COMMANDS=ON \ 
        
                   --update \ 
        
                   --build --parallel 
        
               displayName: Generate compile_commands.json

a few notes:

--build_objc is probably unnecessary. and --use_coreml enabling 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.

cloudhan · 2022-09-21T00:56:44Z

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.

.clang-tidy

justinchuby

Addressed comments

cloudhan

This will greatly improve my code editing experience. Thanks!

justinchuby · 2022-09-21T17:08:01Z

Current command

python tools/ci_build/build.py --build_dir build --update --cmake_extra_defines CMAKE_EXPORT_COMPILE_COMMANDS=ON

justinchuby · 2022-09-21T18:49:16Z

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"!

edgchen1 · 2022-09-21T19:22:43Z

.github/workflows/lint.yml

+        uses: ZedThree/clang-tidy-review@526cbfb043719639f1ebdeedae0cc1eacd219d8f
+        with:
+          token: ${{ secrets.github_token }}
+          build_dir: "build/Debug/compile_commands.json"


should it be the compile_commands.json directory, "build/Debug"?
https://github.com/ZedThree/clang-tidy-review#inputs

no wonder 😅 . thanks!

edgchen1 · 2022-09-21T19:36:28Z

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

justinchuby · 2022-09-26T19:59:23Z

@edgchen1 @cloudhan should

modernize-use-nodiscard be disabled? Otherwise this is ready to merge.

edgchen1 · 2022-09-26T23:59:44Z

@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?

pranavsharma · 2022-09-27T00:05:44Z

@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.

cloudhan · 2022-09-27T01:43:49Z

nondiscard is great. Better to keep it on and fix them from the code side.

justinchuby · 2022-09-28T17:40:07Z

@edgchen1 PTAL

.github/workflows/lint.yml

edgchen1 · 2022-09-28T17:59:40Z

thanks for setting this up!

edgchen1 · 2022-09-28T19:18:22Z

.github/workflows/lint.yml

+            --build_dir build \
+            --update \
+            --cmake_extra_defines CMAKE_EXPORT_COMPILE_COMMANDS=ON
+      - name: Generate ONNX protobuf files


FYI, I just found out about the build.py --target option. so this could possibly be done in the build.py command above, e.g., build.py ... --build --parallel --target onnx_proto

Thanks! For the sake of avoiding running another day of CI tests I merged as is.

cloudhan · 2022-09-29T07:03:10Z

@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.

Co-authored-by: Edward Chen <[email protected]>

justinchuby · 2022-09-29T14:18:09Z

@cloudhan PTAL

Update clang-tidy config to prepare for creating a CI workflow to run clang-tidy. Added clangtidy check in CI Co-authored-by: Edward Chen <[email protected]>

cloudhan reviewed Sep 20, 2022

View reviewed changes

justinchuby marked this pull request as ready for review September 20, 2022 14:48

cloudhan reviewed Sep 21, 2022

View reviewed changes

.clang-tidy Outdated Show resolved Hide resolved

justinchuby commented Sep 21, 2022

View reviewed changes

cloudhan previously approved these changes Sep 21, 2022

View reviewed changes

justinchuby dismissed cloudhan’s stale review via 416076e September 21, 2022 17:24

edgchen1 reviewed Sep 21, 2022

View reviewed changes

justinchuby changed the title ~~Create clang-tidy lint~~ Update clang-tidy config Sep 23, 2022

justinchuby changed the title ~~Update clang-tidy config~~ Create clang-tidy CI Sep 23, 2022

edgchen1 reviewed Sep 28, 2022

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

edgchen1 previously approved these changes Sep 28, 2022

View reviewed changes

justinchuby dismissed edgchen1’s stale review via 4c8a482 September 28, 2022 17:56

justinchuby requested a review from edgchen1 September 28, 2022 17:56

edgchen1 previously approved these changes Sep 28, 2022

View reviewed changes

edgchen1 reviewed Sep 28, 2022

View reviewed changes

justinchuby and others added 16 commits September 29, 2022 14:15

Update lint.yml

361eefb

Update .clang-tidy

76be1e8

Update .clang-tidy

681a16d

Update .clang-tidy

7e64a4e

Update clang-tidy

cbcedc8

Remove

a93d7aa

generate compile_commandsf

d694de5

Pin action SHA

060a090

try this base dir

c225eee

Update lint.yml

83491b1

Update .clang-tidy

0ca3db3

Update lint.yml

6fbf374

Update .clang-tidy

45ac2a1

Update lint.yml

659c238

Update .github/workflows/lint.yml

80495f7

Co-authored-by: Edward Chen <[email protected]>

-readability-uppercase-literal-suffix

b2ab665

justinchuby dismissed edgchen1’s stale review via b2ab665 September 29, 2022 14:17

justinchuby force-pushed the justinchu/clang-tidy branch from 4c8a482 to b2ab665 Compare September 29, 2022 14:17

cloudhan approved these changes Sep 29, 2022

View reviewed changes

justinchuby merged commit 402e199 into main Sep 30, 2022

justinchuby deleted the justinchu/clang-tidy branch September 30, 2022 15:05

linnealovespie pushed a commit that referenced this pull request Sep 30, 2022

Create clang-tidy CI (#12653)

3c0b69a

Update clang-tidy config to prepare for creating a CI workflow to run clang-tidy. Added clangtidy check in CI Co-authored-by: Edward Chen <[email protected]>

cloudhan mentioned this pull request Oct 8, 2022

Fix clang-tidy(cppcoreguidelines-pro-bounds-array-to-pointer-decay) #13241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create clang-tidy CI #12653

Create clang-tidy CI #12653

justinchuby commented Aug 19, 2022 •

edited

Loading

cloudhan Sep 20, 2022

justinchuby Sep 20, 2022

edgchen1 Sep 20, 2022

cloudhan Sep 21, 2022 •

edited

Loading

justinchuby Sep 21, 2022

justinchuby Sep 21, 2022

cloudhan Sep 21, 2022

edgchen1 commented Sep 20, 2022

justinchuby commented Sep 20, 2022

edgchen1 commented Sep 20, 2022

justinchuby commented Sep 20, 2022

edgchen1 commented Sep 21, 2022

cloudhan commented Sep 21, 2022

justinchuby left a comment

cloudhan left a comment

justinchuby commented Sep 21, 2022 •

edited

Loading

justinchuby commented Sep 21, 2022

edgchen1 Sep 21, 2022

justinchuby Sep 23, 2022 •

edited

Loading

edgchen1 commented Sep 21, 2022

justinchuby commented Sep 26, 2022

edgchen1 commented Sep 26, 2022

pranavsharma commented Sep 27, 2022

cloudhan commented Sep 27, 2022

justinchuby commented Sep 28, 2022

edgchen1 commented Sep 28, 2022

edgchen1 Sep 28, 2022

justinchuby Sep 30, 2022

cloudhan commented Sep 29, 2022

justinchuby commented Sep 29, 2022

Create clang-tidy CI #12653

Create clang-tidy CI #12653

Conversation

justinchuby commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudhan Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgchen1 commented Sep 20, 2022

justinchuby commented Sep 20, 2022

edgchen1 commented Sep 20, 2022

justinchuby commented Sep 20, 2022

edgchen1 commented Sep 21, 2022

cloudhan commented Sep 21, 2022

justinchuby left a comment

Choose a reason for hiding this comment

cloudhan left a comment

Choose a reason for hiding this comment

justinchuby commented Sep 21, 2022 • edited Loading

justinchuby commented Sep 21, 2022

Choose a reason for hiding this comment

justinchuby Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

edgchen1 commented Sep 21, 2022

justinchuby commented Sep 26, 2022

edgchen1 commented Sep 26, 2022

pranavsharma commented Sep 27, 2022

cloudhan commented Sep 27, 2022

justinchuby commented Sep 28, 2022

edgchen1 commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudhan commented Sep 29, 2022

justinchuby commented Sep 29, 2022

justinchuby commented Aug 19, 2022 •

edited

Loading

cloudhan Sep 21, 2022 •

edited

Loading

justinchuby commented Sep 21, 2022 •

edited

Loading

justinchuby Sep 23, 2022 •

edited

Loading