Run clang-tidy on modified files only

[esseivaju]

Clang-tidy is quite slow; we can only run it on files updated w.r.t. the target branch during PR to speed it up significantly. During the weekly cron, we still run on the entire codebase.

[sethrj]

Thanks for that... I knew there was a way to do incremental tidying 😄

One more question though... does this tidy code downstream of modified headers? It seems like with incremental but not recursive, you could end up introducing a tidy error that wouldn't get caught until the next cron job.

[github-actions]

Test summary

 3 867 files   5 978 suites   4m 13s ⏱️
 1 609 tests  1 580 ✅ 29 💤 0 ❌
19 896 runs  19 821 ✅ 75 💤 0 ❌

Results for commit 9814284.

♻️ This comment has been updated with latest results.

[esseivaju]

does this tidy code downstream of modified headers?

Good point. With the current configuration, we don't output warnings that would be produced in a header file, though modifying a declaration in a header could introduce a warning in an (un)related source file. That error would only be caught in the cron job. Maybe it's just easier to not do incremental check then?

[sethrj]

Good point. With the current configuration, we don't output warnings that would be produced in a header file, though modifying a declaration in a header could introduce a warning in an (un)related source file. That error would only be caught in the cron job. Maybe it's just easier to not do incremental check then?

I know I've seen at least one GHA that claimed to do an incremental but thorough check... since it seems like this could really be a source of issues (a couple of headers in #1524 did require updating) perhaps we do need it. Incidentally, do you know whether clang-tidy does check header files, or if we need to do something special to make sure inline code in those is clean?

[esseivaju]

I know I've seen at least one GHA that claimed to do an incremental but thorough check... since it seems like this could really be a source of issues (a couple of headers in #1524 did require updating) perhaps we do need it.

Yes, seems like it'd be needed. You'd have to potentially check every cc file transitively including the modified headers..

Incidentally, do you know whether clang-tidy does check header files, or if we need to do something special to make sure inline code in those is clean?

We can configure it to do so by setting HeaderFilterRegex in .clang-tidy. I can look at that if there aren't too many warnings there.

[sethrj]

This is a tricky one... it looks like basically all of the fixes you had in the clang-tidy branch were on .cc files so I think that means that an incremental test would catch most of them. Let's merge it and see what happens?

[esseivaju]

This is a tricky one... it looks like basically all of the fixes you had in the clang-tidy branch were on .cc files so I think that means that an incremental test would catch most of them. Let's merge it and see what happens?

Yes, I think it would catch most of them, and for those that aren't, it wouldn't interfere with the PR workflow until the file containing the missed warning is modified again, which could be updated then if not caught before in the weekly cron.

[sethrj]

LGTM, should we wait until #1534 or merge now? Merge if you want :)

[esseivaju]

Actually, running git diff against develop isn't the best thing because some PR might have been merged in the meantime, we should find the first commit of that PR and run a git diff against that to find the correct set of modified files.

[esseivaju]

LGTM now