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

Decimal #2846

Merged
merged 34 commits into from
Aug 14, 2018
Merged

Decimal #2846

merged 34 commits into from
Aug 14, 2018

Conversation

4ertus2
Copy link
Contributor

@4ertus2 4ertus2 commented Aug 10, 2018

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

template <> struct TypeNumber<Dec128> { static constexpr const size_t value = 18; };

template <typename T>
inline constexpr bool decTrait() { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

There is "template variable" in C++17.
Example:

template <typename T> constexpr bool IsNumber = false;

template <> constexpr bool IsNumber<UInt8> = true;
template <> constexpr bool IsNumber<UInt16> = true;

(Core/Types.h)

namespace std
{

template <> struct is_signed<__int128>
Copy link
Member

Choose a reason for hiding this comment

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

Need to check both with gcc (libstdc++) and clang (libc++).
It can happen that some std library already have type traits for __int128.

template <> constexpr bool decTrait<Dec128>() { return true; }

template <typename T>
inline constexpr bool decBaseTrait() { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

Missing comment.
(that it is true for the types that can be used as a storage type for Decimal)

Copy link
Member

Choose a reason for hiding this comment

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

isValidStorageForDecimal

template <> struct TypeNumber<UInt32> { static constexpr const size_t value = 3; };
template <> struct TypeNumber<UInt64> { static constexpr const size_t value = 4; };
/// 5 reserved for TypeNumber<UInt128>
template <> struct TypeNumber<Float32> { static constexpr const size_t value = 7; };
Copy link
Member

Choose a reason for hiding this comment

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

Unusual order: why UInt then Float then Int? Maybe better UInt then Int than Float.
(neat types first)


template <typename T>
inline constexpr bool decTrait() { return false; }
template <> constexpr bool decTrait<Dec32>() { return true; }
Copy link
Member

Choose a reason for hiding this comment

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

isDecimal function or is_decimal variable will look better,
because now I can't guess "what trait" by looking at name.

@alexey-milovidov alexey-milovidov merged commit a29439c into ClickHouse:master Aug 14, 2018
alexey-milovidov added a commit that referenced this pull request Aug 14, 2018
@alexey-milovidov
Copy link
Member

@4ertus2 a bug has been found: #42451

@4ertus2
Copy link
Contributor Author

4ertus2 commented Oct 19, 2022

@4ertus2 a bug has been found: #42451

shame on me

@alexey-milovidov
Copy link
Member

@4ertus2 Not at all! Actually, you should be proud - the code stands out as very high quality and reliable.

@alexey-milovidov
Copy link
Member

A drawback has been found: #46094

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.

2 participants