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

COMMON: Add numeric limits #3966

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

elasota
Copy link
Contributor

@elasota elasota commented Jun 5, 2022

There are multiple engines using their own min/max definitions because Common doesn't define them. In some cases they might even be buggy (i.e. wintermute.h defines UINT_MAX_VALUE as a signed constant so it's actually -1?)

This adds them to Common.

Copy link
Member

@lephilousophe lephilousophe left a comment

Thanks for this addition which will bring us an architecture agnostic interface for limits.
There is some small changes though to make it cleaner.

struct IntegralNumericLimits<T, true, false> {
static const T max_value = ((T(1) << (sizeof(T) * 8 - 2)) - 1) * 2 + 1;
static const T min_value = -max_value - 1;
static const T lowest_value = min_value;
Copy link
Member

@lephilousophe lephilousophe Jun 5, 2022

Choose a reason for hiding this comment

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

These constants should not be here.
Either private or not here at all to not expose them.
If we mimic C++11 STL, only functions are to be used.

Copy link
Contributor Author

@elasota elasota Jun 5, 2022

Choose a reason for hiding this comment

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

OK, I'll move those into something else... but I do think these values need to be exposed in a way that doesn't need to call a function. It's not just numeric_limits equivalent that's missing, it's also things like INT_MAX and such... so while the functions will work for a lot of cases, they won't work for cases that either need a numeric constant (e.g. template args) or where using a function call would violate code standards by having an on-init function call.

Copy link
Member

@lephilousophe lephilousophe Jun 5, 2022

Choose a reason for hiding this comment

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

Maybe you can try constexpr?

Copy link
Member

@lephilousophe lephilousophe Jun 5, 2022

Choose a reason for hiding this comment

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

INT_MAX and so are really a different thing: they are macros handled by the preprocessor and can be used in #if directives.

common/numeric-limits.h Outdated Show resolved Hide resolved
@criezy
Copy link
Member

@criezy criezy commented Jun 5, 2022

Is there a good reason for not using std::numeric_limits directly?

I know our portability guide says that "the standard C++ library is a big no-no", and while I understand why we want to avoid things like std::string or std::vector, I think we could allow some flexibility here. For example we already use std::initializer_list<T> in common code.

@sev-
Copy link
Member

@sev- sev- commented Jun 12, 2022

I think we can safely use std::numeric_limits

@carlo-bramini
Copy link
Contributor

@carlo-bramini carlo-bramini commented Jul 1, 2022

Excuse me, why not simply using the definitions provided for the stdint types?
If the library supports std::numeric_limits, then I cannot see a good reason why the old good C99 macros shouldn't be also available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants