Explicitly check lib pointer for null #95

brawner · 2020-11-19T23:00:30Z

This showed up as a false positive from clang static analysis. It's because it doesn't realize that gtest ASSERT_* macros will return. Clang does allow for analyzer_noreturn annotations to help it understand, but I believe that's only on functions. Clang does recognize standard assert statements though, so I thought that would be the simplest solution here.

Example

/home/brawner/workspace/ros2_master/src/ros2/rosidl_typesupport/rosidl_typesupport_c/test/test_message_type_support_dispatch.cpp:117:15: warning: Called C++ object pointer is null
  EXPECT_TRUE(lib->has_symbol("test_message_type_support"));
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/brawner/workspace/ros2_master/install/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1969:23: note: expanded from macro 'EXPECT_TRUE'
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \
                      ^~~~~~~~~
/home/brawner/workspace/ros2_master/install/gtest_vendor/src/gtest_vendor/include/gtest/internal/gtest-internal.h:1325:34: note: expanded from macro 'GTEST_TEST_BOOLEAN_'
      ::testing::AssertionResult(expression)) \
                                 ^~~~~~~~~~

Signed-off-by: Stephen Brawner brawner@gmail.com

Signed-off-by: Stephen Brawner <brawner@gmail.com>

brawner · 2020-11-20T00:04:50Z

Force pushed the same change to rosidl_typesupport_cpp

brawner · 2020-11-20T00:06:03Z

Testing --packages-select rosidl_typesupport_c rosidl_typesupport_cpp

Linux
Linux-aarch64
macOS
Windows

clalancette · 2020-11-20T14:08:36Z

rosidl_typesupport_c/test/test_message_type_support_dispatch.cpp

+
  ASSERT_NE(lib, nullptr);
+  // c-style assert is used to avoid false positive of clang static analysis because it can't
+  // follow gtest asserts
+  assert(nullptr != lib);


I believe a somewhat simpler solution here is:

Suggested change

ASSERT_NE(lib, nullptr);

// c-style assert is used to avoid false positive of clang static analysis because it can't

// follow gtest asserts

assert(nullptr != lib);

ASSERT(nullptr != lib);

(I'm not against what you are doing here on principal, but I think it would be a little simpler to just stick with the GTest macros if possible).

Same goes for the change below.

The issue is that scan-build is not following gtest macros to know that it's impossible for lib to be null. I kept the ASSERT_NE in line 120 you'll notice so the assert in 123 is actually impossible to fail and gtest will report the test failure in line 120. But scan-build does acknowledge c-style assert macros, so it will correctly identify that lib can't be null in line 124.

As far as I can find, you can annotate that functions won't return, but I couldn't find the same for macros.

Just out of curiosity, mind changing ASSERT_NE to ASSERT_TRUE(nullptr != lib) and seeing if the analyzer still complains? It seems like there might be some built in magic for it.

https://reviews.llvm.org/D27773
https://clang.llvm.org/doxygen/GTestChecker_8cpp_source.html

Also found:
google/googletest#734