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: Return default value from ‘GetCacheTypeString’ #162
fix: Return default value from ‘GetCacheTypeString’ #162
Conversation
src/utils/list_cpu_features.c
Outdated
@@ -340,6 +340,8 @@ static Node* GetCacheTypeString(CacheType cache_type) { | |||
case CPU_FEATURE_CACHE_PREFETCH: | |||
return CreateConstantString("prefetch"); | |||
} | |||
/* Should never happen */ | |||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use NULL
ref: https://en.cppreference.com/w/c/types/NULL
512e8ed
to
892991f
Compare
src/utils/list_cpu_features.c
Outdated
@@ -340,6 +340,8 @@ static Node* GetCacheTypeString(CacheType cache_type) { | |||
case CPU_FEATURE_CACHE_PREFETCH: | |||
return CreateConstantString("prefetch"); | |||
} | |||
/* Should never happen */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this should never happen I'd rather use __builtin_unreachable()
for clang/gcc and __assume(0)
for MSVC.
Something along these lines:
#if defined(CPU_FEATURES_COMPILER_CLANG) || defined(CPU_FEATURES_COMPILER_GCC)
__builtin_unreachable();
#elif defined(CPU_FEATURES_COMPILER_MSC)
__assume(0);
#else
#error "Unsupported compiler."
#endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, done. thanks, I wasn't aware of those builtins.
Instead of issuing an error for compiler other than MSC,GCC,clang I simply do nothing. In this unusual situation, it is a bit better as compilation may still work if something like -pedantic
is not used.
f0d43fa
to
38f4324
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx! I was about to suggest to add this to include/cpu_features_macros.h
, you read my mind : )
One nit on a missing new line and this is good to go!
The build fails with following message when -Werror and -Werror=return-type are enabled. In function ‘GetCacheTypeString’: error: control reaches end of non-void function [-Werror=return-type] Simple fix is to return explicitly communicate to the compiler that certain block is not reachable.
38f4324
to
efceff6
Compare
Thanks! |
The build fails with following message when -Werror
and -Werror=return-type are enabled.
In function ‘GetCacheTypeString’:
error: control reaches end of non-void function [-Werror=return-type]
Simple fix is to return 0 from that function. The
return value is then checked by an assert in the
AddMapEntry. The assert will only work in debug mode,
but that seems to be enough.