Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The assert macro usage should not produce 3 clang-tidy warnings! #1589

Closed
ClausKlein opened this issue Mar 17, 2020 · 10 comments
Closed

The assert macro usage should not produce 3 clang-tidy warnings! #1589

ClausKlein opened this issue Mar 17, 2020 · 10 comments

Comments

@ClausKlein
Copy link

Is this compiler header problem or a clang-tidy-7 error?

packages/platform/src/RRCP/Connection.cpp|146 col 13| warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
||             assert(getServer() != nullptr);
||             ^
/usr/include/assert.h|95 col 51| note: expanded from macro 'assert'
||       : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION))
||                                                   ^
/usr/include/assert.h|129 col 30| note: expanded from macro '__ASSERT_FUNCTION'
|| #   define __ASSERT_FUNCTION    __extension__ __PRETTY_FUNCTION__
||                                 ^
@jwakely
Copy link
Contributor

jwakely commented Mar 17, 2020

Clang-tidy should suppress that warning. It's unhelpful (it seems to be complaining about converting a compiler-defined null-terminated array into a const char* which is guaranteed to be safe) and users can't do anything about it anyway.

@ClausKlein
Copy link
Author

ClausKlein commented Mar 17, 2020

But a

static_cast<const char *>(__PRETTY_FUNCTION__)

would also help and may be correct, or not?

@jwakely
Copy link
Contributor

jwakely commented Mar 17, 2020

It adds nothing, and it wouldn't help when assert is compiled by C code.

@ClausKlein
Copy link
Author

And what is this cppcoreguidelines-pro-bounds-array-to-pointer-decay check for if it should be suppressed ?

@jwakely
Copy link
Contributor

jwakely commented Mar 17, 2020

N.B. re your issue title, there is no such thing as the std::assert macro, because macros don't use scopes.

The guidelines are for application developers and library writers, not for the implementation. assert is part of the C++ (or C) implementation and so it is useless for clang-tidy to check it and tell you to fix it.

Just because the check doesn't make sense for assert doesn't mean it's useless elsewhere, and it's silly to infer that that's what I meant.

@ClausKlein ClausKlein changed the title The std::assert macro should not produce 3 clang-tidy warnings! The assert macro usage should not produce 3 clang-tidy warnings! Mar 17, 2020
@phillipjohnston
Copy link

I haven't run into this before, which doesn't mean much. I can think of two possible problems:

  1. The build is not correctly including system headers using -isystem
  2. clang-tidy is being invoked with --system-headers

I also see this clang-tidy change, which seems to be relevant and also abandoned: https://reviews.llvm.org/D31130

@ClausKlein
Copy link
Author

I used clang-7 on debian stable, no really actual

@ClausKlein
Copy link
Author

No, it happens with clang-9 on OS X too:

part of compile_commands.json:

{
  "directory": "/Users/clausklein/Workspace/cpp/.build-samples-Debug",
  "command": "/opt/local/bin/clang++  -DGSL_THROW_ON_CONTRACT_VIOLATION  -g   -Wall -Wextra -Wpedantic -Werror -Wno-unused-parameter -Wno-unused-variable -Wno-unknown-warning-option -std=c++17 -o CMakeFiles/ClonableBase.dir/ClonableBase.o -c /Users/clausklein/Workspace/cpp/samples/ClonableBase.cpp",
  "file": "/Users/clausklein/Workspace/cpp/samples/ClonableBase.cpp"
},
Claus-MBP:samples clausklein$ clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 9.0.0svn
  Optimized build with assertions.
  Default target: x86_64-apple-darwin15.6.0
  Host CPU: penryn


/Users/clausklein/Workspace/cpp/samples/ClonableBase.cpp|84 col 5| warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
||     assert(origin.number() == 1);
||     ^
/usr/include/assert.h|93 col 47| note: expanded from macro 'assert'
||     (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0)
||                                               ^
/Users/clausklein/Workspace/cpp/samples/ClonableBase.cpp|91 col 5| warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
||     assert(copy->number() == 1);
||     ^
/usr/include/assert.h|93 col 47| note: expanded from macro 'assert'
||     (__builtin_expect(!(e), 0) ? __assert_rtn(__func__, __FILE__, __LINE__, #e) : (void)0)
||                                               ^

@phillipjohnston
Copy link

As Jonathan mentioned, the proper place to follow up is with the clang-tidy project. I recommend chiming in on this MR: https://reviews.llvm.org/D31130

@ClausKlein
Copy link
Author

ClausKlein commented Mar 17, 2020

ClausKlein referenced this issue in llvm/llvm-project Mar 18, 2020
…nosing array to pointer decay stemming from system macros.

Patch by Breno Rodrigues Guimaraes.

llvm-svn: 298421
krivenko added a commit to pomerol-ed/pomerol that referenced this issue Aug 4, 2021
`prog/args.hxx` and `test/catch2/catch.hpp` are excluded from analysis as 3rd
party libraries.

A few clang-tidy checks have been disabled.

- `modernize-use-trailing-return-type`. A stylistic requirement that would not
  improve the readability of our code.

- `cppcoreguidelines-avoid-magic-numbers`. pomerol is a computational science
  library and naturally contains lots of numerical constants, especially in
  the unit tests.

- `cppcoreguidelines-pro-bounds-constant-array-index`. This one is a bit
  too strict for us, I believe.

- `cppcoreguidelines-pro-type-cstyle-cast` triggers a false positive for every
  use of a macro defined in `mpi.h`. https://reviews.llvm.org/D90835?id=303121.

- `cppcoreguidelines-pro-bounds-array-to-pointer-decay`. Complains about every
  use of `assert()`, see isocpp/CppCoreGuidelines#1589

Besides, cppcheck dislikes initialization syntax of Eigen vectors in some unit
tests. This is a known bug, https://trac.cppcheck.net/ticket/9766.
krivenko added a commit to krivenko/libcommute that referenced this issue Aug 6, 2021
Passing `-DSTATIC_ANALYSIS=ON` to CMake will make it run `clang-tidy`
and `cppcheck` on sources of unit tests and examples.

The installation document has been extended accordingly.

The GitHub Actions workflow will now use the static analysis tools.

Some of `clang-tidy`'s checks have been disaled.

* `modernize-use-trailing-return-type`. I prefer to use the trailing
  return type only when it makes for cleaner code.
* `cppcoreguidelines-avoid-magic-numbers` is generally unhelpful,
  especially in the unit tests.
* `cppcoreguidelines-pro-bounds-array-to-pointer-decay` complains about
  every use of `assert()`, see isocpp/CppCoreGuidelines#1589.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants