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

Fix clang-tidy warnings #1485

Closed
wants to merge 1 commit into from
Closed

Fix clang-tidy warnings #1485

wants to merge 1 commit into from

Conversation

ncihnegn
Copy link

@ncihnegn ncihnegn commented Jan 4, 2019

Clang-tidy gives a warning like this:
/usr/local/include/catch2/catch.hpp:5444:9: warning: Returning null reference [clang-analyzer-core.uninitialized.UndefReturn]
return *IMutableContext::currentContext;

This simple change fixes the warning and the overhead is minimal.

@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #1485 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
- Coverage   80.42%   80.39%   -0.03%     
==========================================
  Files         121      121              
  Lines        3421     3426       +5     
==========================================
+ Hits         2751     2754       +3     
- Misses        670      672       +2

@codecov
Copy link

codecov bot commented Jan 4, 2019

Codecov Report

Merging #1485 into master will decrease coverage by 3.28%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
- Coverage   83.67%   80.39%   -3.28%     
==========================================
  Files         123      121       -2     
  Lines        3380     3426      +46     
==========================================
- Hits         2828     2754      -74     
- Misses        552      672     +120

@horenmar
Copy link
Member

Given how often people complain about this, I agree that it is time to fix it, but I really dislike this specific change, and would prefer to restructure the code differently instead.

However, I ran into some trouble reproducing the warning -- can you post what version of clang-tidy have you used, what did your compilation database look like and so on?

@ncihnegn
Copy link
Author

➜ build git:(master) clang-tidy --version
LLVM (http://llvm.org/):
LLVM version 7.0.1
Optimized build.
Default target: x86_64-apple-darwin18.2.0
Host CPU: skylake

➜ build git:(master) cat ../.clang-tidy
Checks: '*,-bugprone-exception-escape,-clang-analyzer-cplusplus.NewDeleteLeaks,-cppcoreguidelines-pro-bounds-array-to-pointer-decay,-cppcoreguidelines-pro-type-reinterpret-cast,-fuchsia-default-arguments,-fuchsia-overloaded-operator,-fuchsia-trailing-return,-google-default-arguments,-google-runtime-references,-hicpp-no-assembler,-hicpp-no-array-decay,-misc-unused-parameters'

@horenmar
Copy link
Member

Thanks

@yuriy-mochurad
Copy link

I have also faced this issue and would be great to have it fixed.

@poconbhui
Copy link

Adding an assertion before dereferencing the pointer should also silence the warning.

@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

Merging #1485 into master will decrease coverage by 6.12%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1485      +/-   ##
==========================================
- Coverage    86.5%   80.39%   -6.12%     
==========================================
  Files         136      121      -15     
  Lines        5275     3426    -1849     
==========================================
- Hits         4563     2754    -1809     
+ Misses        712      672      -40

@horenmar
Copy link
Member

Fixed in #1735

@horenmar horenmar closed this Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants