build check error: narrowing conversion from 'int' to signed type 'int8_t' (aka 'signed char') is implementation-defined

OpenCyphal / libcanard

A compact implementation of the Cyphal/CAN protocol in C for high-integrity real-time embedded systems

http://opencyphal.org

MIT License

327 stars 192 forks source link

build check error: narrowing conversion from 'int' to signed type 'int8_t' (aka 'signed char') is implementation-defined #178

Closed Linjieqiang closed 2 years ago

Linjieqiang commented 3 years ago

pavel-kirienko commented 3 years ago

I don't see where the conversion is taking place. Which version of Clang-Tidy is this? Libcanard is currently analyzed with Clang Tools version 11.

Linjieqiang commented 3 years ago

I use the segger embedded studio ide to analyze the code. The wiki: https://wiki.segger.com/Static_code_analysis_in_Embedded_Studio

pavel-kirienko commented 3 years ago

Idk this doesn't make sense to me. Can you see what it takes to fix this warning? Maybe I am missing something.

Linjieqiang commented 3 years ago

https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.html

pavel-kirienko commented 3 years ago

I understand what a narrowing conversion is. What I don't understand is where within the expression it occurs, please look into that.

Linjieqiang commented 3 years ago

(int8_t)((-(int8_t)(uint8_t) ~val) - 1) and ((int8_t) val)

Maybe you could have a try?

Linjieqiang commented 3 years ago

When I remove the libcanard\.clang-tidy, the code static check is fine.

---
Checks: >-
  boost-*,
  bugprone-*,
  cert-*,
  clang-analyzer-*,
  cppcoreguidelines-*,
  google-*,
  hicpp-*,
  llvm-*,
  misc-*,
  modernize-*,
  performance-*,
  portability-*,
  readability-*,
  -google-readability-todo,
  -readability-avoid-const-params-in-decls,
  -llvm-header-guard,
CheckOptions:
  - key:   readability-function-cognitive-complexity.Threshold
    value: '99'
WarningsAsErrors: '*'
HeaderFilterRegex: '.*'
AnalyzeTemporaryDtors: false
FormatStyle: file
...

Linjieqiang commented 3 years ago

Maybe you could use the clang 12 to try CI?

thirtytwobits commented 3 years ago

Hmm. I'm not able to reproduce. Can you take a look here:

https://godbolt.org/z/Mdrar3jTo

Linjieqiang commented 3 years ago

Maybe it was a warning, but the inspection conditions are too strict. @thirtytwobits

pavel-kirienko commented 3 years ago

@Linjieqiang it would help if you could isolate the issue to a minimal working example. Also, consider checking the Clang bug tracker to see if this is a known issue. Maybe ask the maintainers about this for good measure unless someone did that already. In the worst case, we may need to add a NOLINT comment on the offending lines to suppress this diagnostic.

Linjieqiang commented 3 years ago

Still haven't found the reason, why don't we use NOLINT to circumvent this problem?

pavel-kirienko commented 3 years ago

We need an MWE first. Please extract the offending fragment into a separate unit and see if you can reproduce the problem there.

Linjieqiang commented 2 years ago

Thanks. @pavel-kirienko

pavel-kirienko commented 2 years ago

I didn't really fix it, the code in question was removed from the library (use Nunavut instead).