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

warnless: simplify type size handling by using sizeof(T) #7181

Closed
wants to merge 1 commit into from

Conversation

@dmitrykos
Copy link
Contributor

@dmitrykos dmitrykos commented Jun 2, 2021

Simplify type size handling by using sizeof(T) and rely on the compiler to define the required signed/unsigned masks.

The idea is to get rid of SIZEOF_XXX defines, which are not needed for this implementation, and a manually created masks which can be generated by the compiler at compile time. In this case type masks are created by the compiler and can never be erroneous for any type.

Such run-time checks if (sizeof(TYPE1) < sizeof(TYPE2)) are removed by the compiler during the optimization stage because the condition is constant, so there is no overhead. MSVC throws warning regarding the constant condition used but it is suppressed by the implementation.

I am using this implementation for many years already without any issue (Linux, Android, Apple iOS, Windows Win32, Windows UWP).

@dmitrykos dmitrykos force-pushed the dmitrykos:simplify_warnless branch from 9ff7963 to a6f7fb5 Jun 2, 2021
Copy link
Member

@bagder bagder left a comment

I appreciate how this makes things simpler!

#endif

if(sizeof(int) < sizeof(size_t))

This comment has been minimized.

@bagder

bagder Jun 2, 2021
Member

This could be made into a preprocessor directive;

#if UINT_MAX < SIZE_T_MAX

This comment has been minimized.

@dmitrykos

dmitrykos Jun 3, 2021
Author Contributor

This could be made into a preprocessor directive;

#if UINT_MAX < SIZE_T_MAX

The reason why I decided using if(sizeof(int) < sizeof(size_t)) instead of preprocessor defines is to have maximum portability (any complier has sizeof, negate and shift operators), if this or that define is missing, like for example SIZE_T_MAX which may be missing in some older MSVC versions, if I am not mistaken, then proposed simplification will run into additional checks for defines existence.

if(sizeof(int) < sizeof(size_t)) is removed by the compiler with any optimization level and therefore has no effect on performance while being effective check in the same time against types inconsistency.

I can of course replace to compile time preprocessor directives but anxious about portability. What do you think about my reasoning?

This comment has been minimized.

@bagder

bagder Jun 3, 2021
Member

Thing is, we use those defines in existing curl source code so we already need to have them available, using them in this file is thus not any added portability issue. I realize a compiler can remove the fixed value condition, but I'm concerned about warnings and that the assert is also a null-operation in non-debug builds so it makes a fixed condition to a blank expression.

This comment has been minimized.

@dmitrykos

dmitrykos Jun 3, 2021
Author Contributor

Ok, I see. I will adjust these checks to the preprocessor directives as proposed then.

#if (SIZEOF_INT < SIZEOF_LONG)
DEBUGASSERT((unsigned long) slnum <= (unsigned long) CURL_MASK_SINT);
#endif
if(sizeof(int) < sizeof(long))

This comment has been minimized.

@bagder

bagder Jun 2, 2021
Member

This could be made into a preprocessor directive:

#if INT_MAX < LONG_MAX
#if (SIZEOF_INT < SIZEOF_SIZE_T)
DEBUGASSERT((size_t) sznum <= (size_t) CURL_MASK_SINT);
#endif
if(sizeof(int) < sizeof(size_t))

This comment has been minimized.

@bagder

bagder Jun 2, 2021
Member

#if UINT_MAX < SIZE_T_MAX
DEBUGASSERT(uznum <= (size_t) CURL_MASK_ULONG);
#ifdef _MSC_VER
# pragma warning(push)
# pragma warning(disable:4127) /* conditional expression is constant */

This comment has been minimized.

@bagder

bagder Jun 2, 2021
Member

If you convert to preprocessor conditions, maybe this pragma can be removed?

@dmitrykos dmitrykos force-pushed the dmitrykos:simplify_warnless branch from a6f7fb5 to 6a6de19 Jun 3, 2021
@dmitrykos
Copy link
Contributor Author

@dmitrykos dmitrykos commented Jun 3, 2021

@bagder I made changes but had to introduce a missing SSIZE_T_MAX in curl_setup.h.

…piler to define the required signed/unsigned mask
@dmitrykos dmitrykos force-pushed the dmitrykos:simplify_warnless branch from 6a6de19 to 5bfd59c Jun 3, 2021
@bagder
bagder approved these changes Jun 4, 2021
@bagder
Copy link
Member

@bagder bagder commented Jun 4, 2021

Thanks!

@bagder bagder closed this in e4662ad Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants