Added default case in switch to avoid warning #716

amallia · 2018-11-09T22:39:51Z

warning: missing return statement at end of non-void function "benchmark::GetTimeUnitString"
warning: missing return statement at end of non-void function "benchmark::GetTimeUnitMultiplier"

nvcc: NVIDIA (R) Cuda compiler driver
Copyright (c) 2005-2016 NVIDIA Corporation
Built on Tue_Jan_10_13:22:03_CST_2017
Cuda compilation tools, release 8.0, V8.0.61

warning: missing return statement at end of non-void function "benchmark::GetTimeUnitString" warning: missing return statement at end of non-void function "benchmark::GetTimeUnitMultiplier"

googlebot · 2018-11-09T22:39:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

amallia · 2018-11-09T22:43:26Z

I signed it!

googlebot · 2018-11-09T22:43:28Z

CLAs look good, thanks!

coveralls · 2018-11-09T23:00:17Z

Coverage remained the same at 89.224% when pulling 5cdbab7 on amallia:patch-1 into a9b31c5 on google:master.

AppVeyorBot · 2018-11-09T23:59:26Z

✅ Build benchmark 1551 completed (commit 72f4f79 by @amallia)

LebedevRI · 2018-11-10T06:01:32Z

include/benchmark/benchmark.h

@@ -1518,6 +1518,7 @@ inline const char* GetTimeUnitString(TimeUnit unit) {
    case kMicrosecond:
      return "us";
    case kNanosecond:
+    default:	
      return "ns";
  }
  BENCHMARK_UNREACHABLE();


This is the line that is supposed to fix it.
I think you should instead make sure it expands to some this is unreachable your compiler understands.

The problem is that now all the compilers understand BENCHMARK_UNREACHABLE();

benchmark/include/benchmark/benchmark.h

Line 253 in d8c0f27

#define BENCHMARK_UNREACHABLE() ((void)0)

Have you tried changing that to

#if defined(__GNUC__) || __has_builtin(__builtin_unreachable) || defined(WHATEVER_NVCC_MACRO) #define BENCHMARK_UNREACHABLE() __builtin_unreachable() #elif defined(_MSC_VER) #define BENCHMARK_UNREACHABLE() __assume(false) #else #define BENCHMARK_UNREACHABLE() ((void)0) #endif

?

I believe since __has_builtin(__builtin_unreachable) is false, it should not make any difference...

Yes @brenoguim you are correct

Option #2 introduces warnings in MSVC :( https://ci.appveyor.com/project/google/benchmark/builds/20198554/job/ub5ulj85wb67ym3o

Does std::abort work? Or any other noreturn function?

Disabling that warning on the whole code can be harmful.

Not on the whole of the code base, just for that function. But I'm strongly opposed to adding a default case.

We've discussed this before, and we've decided on this direction.

Yeah, adding the default should not be an option.

I like the std::abort suggestion. Writing the BENCHMARK_UNREACHABLE last fallback as std::abort would keep the good code generation for the targets that support it, and silence the warning for the others.

@brenoguim

Update based on suggestion #2 from @brenoguim

AppVeyorBot · 2018-11-10T17:58:14Z

✅ Build benchmark 1552 completed (commit 5e2e0bf by @amallia)

LebedevRI · 2018-11-24T16:14:43Z

On which compiler is nvcc based? Does it really not have __builtin_unreachable() or some analog of it?

dominichamon · 2019-03-18T12:25:35Z

do {std::abort(); } while(0) should be a good replacement for unreachable code in any compiler, i think.

googlebot added the cla: no label Nov 9, 2018

googlebot added cla: yes and removed cla: no labels Nov 9, 2018

LebedevRI requested changes Nov 10, 2018

View changes

Removed default fro switches

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

5cdbab7

Update based on suggestion #2 from @brenoguim

LebedevRI added the incomplete label May 1, 2019

google / benchmark

Added default case in switch to avoid warning #716

Added default case in switch to avoid warning #716

amallia commented Nov 9, 2018 •

edited

googlebot commented Nov 9, 2018

amallia commented Nov 9, 2018

googlebot commented Nov 9, 2018

coveralls commented Nov 9, 2018 •

edited

AppVeyorBot commented Nov 9, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

AppVeyorBot commented Nov 10, 2018

LebedevRI commented Nov 24, 2018

dominichamon commented Mar 18, 2019

google / benchmark

Added default case in switch to avoid warning #716

Added default case in switch to avoid warning #716

Conversation

amallia commented Nov 9, 2018 • edited

googlebot commented Nov 9, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

amallia commented Nov 9, 2018

googlebot commented Nov 9, 2018

coveralls commented Nov 9, 2018 • edited

AppVeyorBot commented Nov 9, 2018

This comment has been minimized.

LebedevRI Nov 10, 2018 Collaborator

This comment has been minimized.

amallia Nov 10, 2018 Author

This comment has been minimized.

amallia Nov 10, 2018 Author

This comment has been minimized.

LebedevRI Nov 10, 2018 Collaborator

This comment has been minimized.

amallia Nov 10, 2018 Author

This comment has been minimized.

amallia Nov 10, 2018 Author

This comment has been minimized.

brenoguim Nov 10, 2018

This comment has been minimized.

EricWF Nov 10, 2018 Contributor

This comment has been minimized.

EricWF Nov 10, 2018 Contributor

This comment has been minimized.

brenoguim Nov 10, 2018

AppVeyorBot commented Nov 10, 2018

LebedevRI commented Nov 24, 2018

dominichamon commented Mar 18, 2019

amallia commented Nov 9, 2018 •

edited

coveralls commented Nov 9, 2018 •

edited

LebedevRI Nov 10, 2018
Collaborator

amallia Nov 10, 2018
Author

amallia Nov 10, 2018
Author

LebedevRI Nov 10, 2018
Collaborator

amallia Nov 10, 2018
Author

amallia Nov 10, 2018
Author

EricWF Nov 10, 2018
Contributor

EricWF Nov 10, 2018
Contributor