Clang tidy not picking up all diagnostics in some code

[matanlurey]

I see a warning in my IDE (via clangd) in shell/platform/android/android_display.cc:

The parameter 'jni_facade' is copied for each invocation but only used as a const reference; consider making it a const referenceclang-tidy[performance-unnecessary-value-param](https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html)
param jni_facade
Type: std::shared_ptr<PlatformViewAndroidJNI>

// In AndroidDisplay::AndroidDisplay
std::shared_ptr<PlatformViewAndroidJNI> jni_facade

However, it isn't linted (either on CI or locally, i.e. with ./ci/clang_tidy.sh --variant android_debug_unopt_arm64.

@christopherfujino and I suspected maybe my configuration locally is different, there are different binaries, a bug in the rule, etc, but interestingly enough in another file (impeller/renderer/shader_library.cc) if I remove the opt-out (// NOLINT(performance-unnecessary-value-param)) it does trigger:

./ci/clang_tidy.sh --variant android_debug_unopt_arm64

🔶 linting flutter/impeller/renderer/shader_library.cc
🔶 linting flutter/shell/platform/android/android_display.cc
[0:04] Jobs:  50% done,   0/2   completed,  1 in progress,   0 pending,   1 failed.    
[0:04] Still running: [clang-tidy on /Users/matanl/Developer/engine/src/flutter/shell/platform/android/android_display.cc]
❌ Failures for clang-tidy on /Users/matanl/Developer/engine/src/flutter/impeller/renderer/shader_library.cc:
/Users/matanl/Developer/engine/src/flutter/impeller/renderer/shader_library.cc:18:26: error: the parameter 'callback' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
   18 |     RegistrationCallback callback) {
      |                          ^
Suppressed 1920 warnings (1894 in non-user code, 26 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[0:13] Jobs: 100% done,   1/2   completed,  0 in progress,   0 pending,   1 failed.    

Lint problems found

Even more interesting, the same lint will get flagged if I add another case of it in the same file:

// An example of a method that violates performance-unnecessary-value-param
void example(std::shared_ptr<int> a) {}

🔶 linting flutter/shell/platform/android/android_display.cc
[0:12] Jobs: 100% done,   0/1   completed,  0 in progress,   0 pending,   1 failed.    
❌ Failures for clang-tidy on /Users/matanl/Developer/engine/src/flutter/shell/platform/android/android_display.cc:
/Users/matanl/Developer/engine/src/flutter/shell/platform/android/android_display.cc:10:35: error: the parameter 'a' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param,-warnings-as-errors]
   10 | void example(std::shared_ptr<int> a) {}
      |                                   ^
      |              const               &
Suppressed 2717 warnings (2687 in non-user code, 30 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error

Lint problems found.

This is also linted in the same file:

// An example of a method that violates performance-unnecessary-value-param
class Foo {
 public:
  explicit Foo(std::shared_ptr<PlatformViewAndroidJNI> jni_facade);
};

Foo::Foo(std::shared_ptr<PlatformViewAndroidJNI> jni_facade) {}

So either there is something specific about why AndroidDisplay::AndroidDisplay isn't linted, or its a clang-tidy bug.

[bc-lee]

clang-tidy won't generate a warning for performance-unnecessary-value-param if std::move is used.

The constructor in flutter/shell/platform/android/android_display.cc moves the parameter jni_facade into the member variable jni_facade_, so the warning probably won't be generated in this case.

To confirm if you're using the project's embedded clang-tidy binary (flutter/buildtools/{os}-{arch}/clang/bin/clang-tidy) , you can specify the Clang-Tidy binary using the C_Cpp.codeAnalysis.clangTidy.path setting in VSCode or Settings | Languages & Frameworks | C/C++ in CLion.

[matanlurey]

Huh, that sort of begs that my VSCode configuration is borked some how (it's using ${workspaceFolder}/buildtools/mac-arm64/clang/bin/clangd, which I assume is synced with clang-tidy)?

Makes sense, thanks for flagging.