Fix clang-tidy errors

[armandas]

We are using clang-tidy for static analysis and we get the following error

error: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage [clang-analyzer-core.uninitialized.Assign,-warnings-as-errors]
    _flags &= KEY_IS_OWNED;
           ^

in this function:

  void setType(uint8_t t) {
    _flags &= KEY_IS_OWNED;
    _flags |= t;
  }

This could be resolved by adding the SYSTEM keyword to target_include_directories, but I'm aware that GCC prior to version 9 wraps all system headers in extern "C", breaking the C++ libraries.

Maybe we could add an option to specify if the SYSTEM keyword should be added? Any other ideas?

[bblanchon]

Hi,

Thank you for reporting this issue 👍

I'm surprised that the static analysis finds an uninitialized value since I already run all sorts of sanitizers, but maybe it's time to add a clang-tidy job to the CI...

I'm already setting the #pragma clang system_header in ArduinoJson.hpp.
This should be equivalent to the SYSTEM flag in CMake, right?

Best regards,
Benoit

[armandas]

Hi. Thanks for the quick reply.

It appears to me that clang pragmas do not affect clang-tidy. I have tried on clang-tidy versions 10 and 11.

[armandas]

A quick update and a correction...

After messing around with adding the SYSTEM keyword in CMake, it still did not solve the clang-tidy error.

How about adding NOLINT comment on the offending line? If this is acceptable, I would be happy to create a PR.

[bblanchon]

I will not add a new line (even a comment) if we don't reproduce the issue in the CI first.
What would really help me would be a PR that ads clang-tidy to the CI... do you think you can do that?

[armandas]

OK, fair enough. I have created a test branch here. You can see the CI run here. Do you think this OK to start with?

Eventually, the CI should be configured to fail on clang-tidy warnings, but before that, someone needs to go through all the reported issues and either add them to ignore list or fix them.

The configuration is done using .clang-tidy file (https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics).

[bblanchon]

Yes, this is a good start 👍

As you said, we need to figure out how to make the build fail.
Let's also ignore the warnings coming from the test suite, like the ones about strcpy().

Then, I should review each warning and fix them myself.
From what I can see, there are 9 warnings:

src/ArduinoJson/Json/JsonDeserializer.hpp:29:9: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Json/JsonSerializer.hpp:19:36: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Json/PrettyJsonSerializer.hpp:19:65: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Json/TextFormatter.hpp:23:44: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Json/Utf16.hpp:34:32: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Json/Utf16.hpp:53:5: warning: Undefined or garbage value returned to caller [clang-analyzer-core.uninitialized.UndefReturn]
src/ArduinoJson/Memory/MemoryPool.hpp:40:5: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc]
src/ArduinoJson/StringStorage/StringCopier.hpp:13:42: warning: 3 uninitialized fields at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
src/ArduinoJson/Variant/VariantData.hpp:322:12: warning: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage [clang-analyzer-core.uninitialized.Assign]

Most of them are due to uninitialized fields that I purposely optimized out to reduce the code size (the fields aren't initialized in the constructor but are initialized by function calls).
Yet, I want to review each one in detail to make sure I'm not missing something.

[armandas]

Ok, I'll make the changes and create a PR today.

…

On Thu, 3 Jun 2021, 16:55 Benoît Blanchon, ***@***.***> wrote: Yes, this is a good start 👍 As you said, we need to figure out how to make the build fail. Let's also ignore the warnings coming from the test suite, like the ones about strcpy(). Then, I should review each warning and fix them myself. From what I can see, there are 9 warnings: src/ArduinoJson/Json/JsonDeserializer.hpp:29:9: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Json/JsonSerializer.hpp:19:36: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Json/PrettyJsonSerializer.hpp:19:65: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Json/TextFormatter.hpp:23:44: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Json/Utf16.hpp:34:32: warning: 1 uninitialized field at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Json/Utf16.hpp:53:5: warning: Undefined or garbage value returned to caller [clang-analyzer-core.uninitialized.UndefReturn] src/ArduinoJson/Memory/MemoryPool.hpp:40:5: warning: Use of memory after it is freed [clang-analyzer-unix.Malloc] src/ArduinoJson/StringStorage/StringCopier.hpp:13:42: warning: 3 uninitialized fields at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject] src/ArduinoJson/Variant/VariantData.hpp:322:12: warning: The left expression of the compound assignment is an uninitialized value. The computed value will also be garbage [clang-analyzer-core.uninitialized.Assign] Most of them are due to uninitialized fields that I purposely optimized out to reduce the code size (the fields aren't initialized in the constructor but are initialized by function calls). Yet, I want to review each one in detail to make sure I'm not missing something. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1574 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOIFLYXZKEFTLOUCFDIQTTQ4YOPANCNFSM45TPBPAQ> .

[bblanchon]

All the checks should pass now.
Thank you very much for the PR, @armandas.

[bblanchon]

The fix is included in ArduinoJson 6.18.1