Use C++14 number separators for large numbers, lowercase for hexadecimal #44224

Calinou · 2020-12-09T15:11:07Z

This makes large numbers easier to read.

Decimal numbers are split starting from 7 digits (in groups of 3). Hexadecimal numbers are split starting from 8 characters (in groups of 4, except ASCII characters which are split in groups of 2).

Hexadecimal numbers were also converted to lowercase for better readability when mixed with numbers.

For reference, this was used for converting hexadecimal numbers to lowercase:

VS Code regex for searching (case-sensitive): 0x([0-9a-FA-F']+)
Replace with: 0x\L$1
Files to exclude: thirdparty,*.po,*.pot (+ VCS ignores)

barbaros83 · 2020-12-09T16:44:17Z

is this also possible in gdscript ? if not, can it be implemented ?

Calinou · 2020-12-09T16:51:42Z

is this also possible in gdscript ? if not, can it be implemented ?

Yes, this is suppported since Godot 3.0. 123_456_789, 0xFFFF_0000 and 0b1001_1111 all work (binary literals were added in 3.2).

aaronfranke · 2020-12-09T20:54:10Z

I think it would be better to use scientific notation in many of these cases: https://www.cplusplus.com/reference/ios/scientific/

I think this is only for floats/doubles, but it can still be assigned to an int with implicit truncation.

This makes large numbers easier to read. Decimal numbers are split starting from 7 digits (in groups of 3). Hexadecimal numbers are split starting from 8 characters (in groups of 4, except ASCII characters which are split in groups of 2). Hexadecimal numbers were also converted to lowercase for better readability when mixed with numbers.

Calinou · 2022-05-03T21:59:38Z

Rebased against latest master and tested on demo projects, it works successfully.

aaronfranke · 2022-05-03T23:44:58Z

core/input/input.cpp

@@ -187,7 +187,7 @@ void Input::get_argument_options(const StringName &p_function, int p_idx, List<S
 void Input::VelocityTrack::update(const Vector2 &p_delta_p) {
 	uint64_t tick = OS::get_singleton()->get_ticks_usec();
 	uint32_t tdiff = tick - last_tick;
-	float delta_t = tdiff / 1000000.0;
+	float delta_t = tdiff / 1'000'000.0;


For floating-point literals, it's worth considering the exponential form 1e6 instead.

EDIT: Oh, I already suggested this in 2020 :P

lawnjelly · 2022-05-04T07:28:40Z

Maybe it's just me, but this looks awful, prefer the old way! 😁
Seems a bit like a solution in search of a problem... but am happy with what the majority want to go for.

neikeq · 2022-05-04T12:57:30Z

Seems a bit like a solution in search of a problem...

If you're referring to the number separators, they solve a well defined problem. When reading code, you can't tell right away what number 10000000 is. You have to count the digits. With 10'000'000 it's clear right away.

I do think it makes hex values look worse and it adds little benefit there. I would only use it for hex values larger than 32-bits.

akien-mga · 2022-05-04T13:09:42Z

core/io/file_access_encrypted.h

@@ -33,7 +33,7 @@

 #include "core/io/file_access.h"

-#define ENCRYPTED_HEADER_MAGIC 0x43454447
+#define ENCRYPTED_HEADER_MAGIC 0x43'45'44'47


This isn't consistent with 0xffff'ffff used in other places.

I'm not sure either makes sense to me, if anything I'd put separators every byte (so none here, only when there's more than 8 bits).

Two hex digits are one byte. One hex digit is 4 bits. So this is already separators every byte.

Ah indeed! Then I guess it makes sense, though it still looks a bit overkill to me to separate every 2 or even 4 hex digits.

I guess it's best do this on a case-by-case basis. I'll revert changes made to hexadecimal numbers, except in two situations:

Numbers with more than 8 hexadecimal digits (will be separated every 4 digits).

Numbers that represent binary file magic (will be separated every 2 digits).

Does this sound good?

Is it useful? Splitting it into bytes doesn't make in more readable, probably better to add a // ASCII "GDEC" comment and uses 4 digit splits, like other places do.

akien-mga · 2022-05-04T13:12:38Z

core/math/audio_frame.h

 }

-static const float AUDIO_PEAK_OFFSET = 0.0000000001f;
+static const float AUDIO_PEAK_OFFSET = 0.000'000'000'1f;


1e-10 would be better here IMO.

akien-mga · 2022-05-04T13:14:30Z

core/math/basis.cpp

+	const static real_t s_c3 = 0.946'174'695'75; // (3*sqrt(5))/(4*sqrt(pi))
+	const static real_t s_c4 = -0.315'391'565'25; // (-sqrt(5))/(4*sqrt(pi))
+	const static real_t s_c5 = 0.546'274'215'29; // (sqrt(15))/(4*sqrt(pi))

-	const static real_t s_c_scale = 1.0 / 0.91529123286551084;
-	const static real_t s_c_scale_inv = 0.91529123286551084;
+	const static real_t s_c_scale = 1.0 / 0.915'291'232'865'510'84;
+	const static real_t s_c_scale_inv = 0.915'291'232'865'510'84;

-	const static real_t s_rc2 = 1.5853309190550713 * s_c_scale;
+	const static real_t s_rc2 = 1.585'330'919'055'071'3 * s_c_scale;


It looks pretty weird to me on arbitrary decimal numbers.

IMO using number separators makes sense for things like 10'000'000, but for the arbitrary result of a computation, I don't think it's useful. This is copy pasted output from a calculator/terminal and the only way to proofread it is to just copy paste a new value on top and check the diff. It's not something that you have to read by intermediate units.

akien-mga · 2022-05-04T13:15:20Z

core/math/math_defs.h

@@ -56,7 +56,7 @@
 #define UNIT_EPSILON 0.001
 #endif

-#define USEC_TO_SEC(m_usec) ((m_usec) / 1000000.0)
+#define USEC_TO_SEC(m_usec) ((m_usec) / 1'000'000.0)


Suggested change

#define USEC_TO_SEC(m_usec) ((m_usec) / 1'000'000.0)

#define USEC_TO_SEC(m_usec) ((m_usec) / 1e6)

akien-mga · 2022-05-04T13:16:15Z

core/math/math_funcs.cpp

@@ -86,7 +86,7 @@ int Math::step_decimals(double p_step) {
 // Only meant for editor usage in float ranges, where a step of 0
 // means that decimal digits should not be limited in String::num.
 int Math::range_step_decimals(double p_step) {
-	if (p_step < 0.0000000000001) {
+	if (p_step < 0.000'000'000'000'1) {


Suggested change

if (p_step < 0.000'000'000'000'1) {

if (p_step < 1e-13) {

akien-mga · 2022-05-04T13:17:09Z

core/math/math_funcs.h

@@ -411,7 +410,7 @@ class Math {
 			uint64_t i;
 		} u;
 		u.d = g;
-		u.i &= (uint64_t)9223372036854775807ll;
+		u.i &= (uint64_t)9'223'372'036'854'775'807ll;


Could maybe be replaced by a constexpr to calculate it at compile time instead of hardcoding.

Or, ~(1ll << 63), or 7F'FF'FF'FF'FF'FF'FF'FF

reduz · 2022-07-29T10:16:54Z

Not particularly against this, but for those like me used to low level, hex in lowercase if unreadable. Hex should always be uppercase.

Calinou requested review from aaronfranke, AndreaCatania, BastiaanOlij, hpvb, neikeq, vnen and a team as code owners December 9, 2020 15:11

Calinou added enhancement topic:codestyle labels Dec 9, 2020

Calinou added this to the 4.0 milestone Dec 9, 2020

Calinou removed request for a team, aaronfranke, BastiaanOlij, hpvb, vnen, neikeq and AndreaCatania December 9, 2020 15:11

Calinou force-pushed the use-cpp14-number-separators branch from 60e068d to 09571e0 Compare May 3, 2022 21:59

Calinou requested review from a team as code owners May 3, 2022 21:59

Calinou removed request for a team May 3, 2022 21:59

aaronfranke reviewed May 3, 2022

View reviewed changes

akien-mga reviewed May 4, 2022

View reviewed changes

akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use C++14 number separators for large numbers, lowercase for hexadecimal #44224

Use C++14 number separators for large numbers, lowercase for hexadecimal #44224

Calinou commented Dec 9, 2020 •

edited

barbaros83 commented Dec 9, 2020 •

edited

Calinou commented Dec 9, 2020 •

edited

aaronfranke commented Dec 9, 2020

Calinou commented May 3, 2022 •

edited

aaronfranke May 3, 2022 •

edited

lawnjelly commented May 4, 2022

neikeq commented May 4, 2022

akien-mga May 4, 2022

aaronfranke May 4, 2022

akien-mga May 4, 2022

Calinou May 4, 2022 •

edited

bruvzg May 4, 2022

akien-mga May 4, 2022

akien-mga May 4, 2022

akien-mga May 4, 2022

akien-mga May 4, 2022

akien-mga May 4, 2022

Faless May 5, 2022

reduz commented Jul 29, 2022

	#define USEC_TO_SEC(m_usec) ((m_usec) / 1'000'000.0)
	#define USEC_TO_SEC(m_usec) ((m_usec) / 1e6)

Use C++14 number separators for large numbers, lowercase for hexadecimal #44224

Are you sure you want to change the base?

Use C++14 number separators for large numbers, lowercase for hexadecimal #44224

Conversation

Calinou commented Dec 9, 2020 • edited

barbaros83 commented Dec 9, 2020 • edited

Calinou commented Dec 9, 2020 • edited

aaronfranke commented Dec 9, 2020

Calinou commented May 3, 2022 • edited

aaronfranke May 3, 2022 • edited

Choose a reason for hiding this comment

lawnjelly commented May 4, 2022

neikeq commented May 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Calinou May 4, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reduz commented Jul 29, 2022

Calinou commented Dec 9, 2020 •

edited

barbaros83 commented Dec 9, 2020 •

edited

Calinou commented Dec 9, 2020 •

edited

Calinou commented May 3, 2022 •

edited

aaronfranke May 3, 2022 •

edited

Calinou May 4, 2022 •

edited