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

kcapi-kdf: Move code to fix #83

Closed
wants to merge 2 commits into from
Closed

kcapi-kdf: Move code to fix #83

wants to merge 2 commits into from

Conversation

kraj
Copy link
Contributor

@kraj kraj commented Nov 17, 2019

Fixes clang build
unused function '_bswap32' [-Werror,-Wunused-function]

Signed-off-by: Khem Raj raj.khem@gmail.com

@WOnder93
Copy link
Contributor

How about moving also the whole #ifdef __HAVE_BUILTIN_BSWAP32__ block along with these functions? Then you could merge it with the # define be_bswap32(x) _swap32(x) and have just:

# ifdef __HAVE_BUILTIN_BSWAP32__
#  define be_bswap32(x) (uint32_t)__builtin_bswap32((uint32_t)(x))
# else
#  define be_bswap32(x) _bswap32(x)
# endif

Actually, you should then also move the inline functions straight under the # else, otherwise you could theoretically still get the warning with compilers that don't have __builtin_bswap32().

@kraj kraj force-pushed the master branch 2 times, most recently from a028eaf to 00c1654 Compare November 18, 2019 17:14
@kraj
Copy link
Contributor Author

kraj commented Nov 18, 2019

@WOnder93 please check the latest version of patch, it should address your comments.

lib/kcapi-kdf.c Outdated Show resolved Hide resolved
@kraj kraj force-pushed the master branch 2 times, most recently from 36637cd to 7b5dd67 Compare November 21, 2019 05:20
@WOnder93
Copy link
Contributor

Looks good now! (One minor nit: I would leave the first letter of the error message uppercase, but it's not a big deal :)

Fixes clang build
unused function '_bswap32' [-Werror,-Wunused-function]

Signed-off-by: Khem Raj <raj.khem@gmail.com>
clang pretends to be gcc 4.2.1 so GCC_VERSION macro will decide that
__builtin_bswap32 is not supported on clang, whereas in reality it might
so its better to add a check for enquiring clang if it supports
__builtin_bswap32 or not

Signed-off-by: Khem Raj <raj.khem@gmail.com>
@kraj
Copy link
Contributor Author

kraj commented Nov 21, 2019

Looks good now! (One minor nit: I would leave the first letter of the error message uppercase, but it's not a big deal :)

makes sense, done, please re-review

Copy link
Contributor

@WOnder93 WOnder93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smuellerDD
Copy link
Owner

Thank you very much. Applied. (sorry for the delay, but the LRNG development took my time)

@smuellerDD smuellerDD closed this Nov 23, 2019
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

Successfully merging this pull request may close these issues.

None yet

3 participants