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

[Strings] Add more number formatters #2873

Merged
merged 8 commits into from
Mar 4, 2023
Merged

Conversation

Kinglykrab
Copy link
Contributor

Notes

  • Adds Strings::ToUnsignedInt for uint32 support.
  • Adds Strings::ToBigInt for int64 support.
  • Adds Strings::ToUnsignedBigInt for uint64 support.
  • Adds Strings::ToFloat for float support.
  • Replaces all std::stoi references with Strings::ToInt.
  • Replaces all atoi references with Strings::ToInt.
  • Replaces all std::stoul references with Strings::ToUnsignedInt.
  • Replaces all atoul references with Strings::ToUnsignedInt.
  • Replaces all std::stoll references with Strings::ToBigInt.
  • Replaces all atoll references with Strings::ToBigInt.
  • Replaces all std::stoull references with Strings::ToUnsignedBigInt.
  • Replaces all atoull references with Strings::ToUnsignedBigInt.
  • Replaces all std::stof references with Strings::ToFloat.

# Notes
- Adds `Strings::ToUnsignedInt` for `uint32` support.
- Adds `Strings::ToBigInt` for `int64` support.
- Adds `Strings::ToUnsignedBigInt` for `uint64` support.
- Adds `Strings::ToFloat` for `float` support.
- Replaces all `std::stoi` references with `Strings::ToInt`.
- Replaces all `atoi` references with `Strings::ToInt`.
- Replaces all `std::stoul` references with `Strings::ToUnsignedInt`.
- Replaces all `atoul` references with `Strings::ToUnsignedInt`.
- Replaces all `std::stoll` references with `Strings::ToBigInt`.
- Replaces all `atoll` references with `Strings::ToBigInt`.
- Replaces all `std::stoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `atoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `std::stof` references with `Strings::ToFloat`.
@Akkadius
Copy link
Member

Looks great, clean.

Gonna need a rebase and we should at least do some respectable surface level testing

Kinglykrab and others added 4 commits February 14, 2023 17:35
- Adds `Strings::ToUnsignedInt` for `uint32` support.
- Adds `Strings::ToBigInt` for `int64` support.
- Adds `Strings::ToUnsignedBigInt` for `uint64` support.
- Adds `Strings::ToFloat` for `float` support.
- Replaces all `std::stoi` references with `Strings::ToInt`.
- Replaces all `atoi` references with `Strings::ToInt`.
- Replaces all `std::stoul` references with `Strings::ToUnsignedInt`.
- Replaces all `atoul` references with `Strings::ToUnsignedInt`.
- Replaces all `std::stoll` references with `Strings::ToBigInt`.
- Replaces all `atoll` references with `Strings::ToBigInt`.
- Replaces all `std::stoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `atoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `std::stof` references with `Strings::ToFloat`.
@Akkadius
Copy link
Member

I'll take a look at this one later

@Akkadius
Copy link
Member

Looked through this all, fairly straightforward.

Have you tested?

I would like to test it and we should probably get at least one other person to also look at it since this is a large surface area

@Kinglykrab
Copy link
Contributor Author

Looked through this all, fairly straightforward.

Have you tested?

I would like to test it and we should probably get at least one other person to also look at it since this is a large surface area

Yeah, I tested, logged in, ran around, bought items, casted spells, just random stuff since it touches quite a large base.

Copy link
Contributor

@Aeadoin Aeadoin left a comment

Choose a reason for hiding this comment

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

Looks good to me as well.

@Aeadoin
Copy link
Contributor

Aeadoin commented Mar 3, 2023

I did some testing this this as well, I'm around this weekend if we merge and there's issues (don't think there will be, but this does cover a large scope)

@Aeadoin Aeadoin requested a review from Akkadius March 3, 2023 15:22
Copy link
Contributor

@Aeadoin Aeadoin left a comment

Choose a reason for hiding this comment

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

Going to run some benchmarking on this

@Aeadoin
Copy link
Contributor

Aeadoin commented Mar 4, 2023

Initial Benchmarking Tests:

Compilation Mode: Debug

String Value of "1111753784"

image

Bencmarking Test 2

Compile Mode: RelwithDebInfo

String Value of "1111753784"

image

Benchmarking Test 3 - Fixed Float testing (previous tests can be discarded for Floats)

Compile Mode: Debug

image

Above tests show a very minimal performance impact especially with increase in "correctness"

Try-Catch Block Details - https://stackoverflow.com/questions/16784601/does-try-catch-block-decrease-performance

Will run some additional test cases in addition to these.

@Akkadius
Copy link
Member

Akkadius commented Mar 4, 2023

Worked with @Aeadoin @Kinglykrab

There was concern that try / catch has performance impact implications. This is not a bad call-out, however it entirely depends and this thread does a decent job of explaining https://stackoverflow.com/questions/16784601/does-try-catch-block-decrease-performance

  • Removed try / catch for simpler IsNumber IsFloat validation functions
  • Removed any non IsNumber variants for use of simply IsNumber
  • Added unit tests to cover scenarios for IsNumber IsFloat
  • Fixed test suite pipeline to just use stdout instead of dumping to a file (which didn't have anything anyways)
  • Added CLI command test:string-benchmark so we can use this benchmark in later testing

PR as is (unoptimized, debug)

image

PR as is (O1)

image

Simplifications to IsNumber / IsFloat (O1)

image

Tests

	void TestIsFloat() {
		TEST_ASSERT_EQUALS(Strings::IsFloat("0.23424523"), true);
		TEST_ASSERT_EQUALS(Strings::IsFloat("12312312313.23424523"), true);
		TEST_ASSERT_EQUALS(Strings::IsFloat("12312312313"), true);
		TEST_ASSERT_EQUALS(Strings::IsFloat(".234234"), true);
		TEST_ASSERT_EQUALS(Strings::IsFloat(".234234f"), false);
		TEST_ASSERT_EQUALS(Strings::IsFloat("Johnson"), false);
	}

	void TestIsNumber() {
		TEST_ASSERT_EQUALS(Strings::IsNumber("0.23424523"), false);
		TEST_ASSERT_EQUALS(Strings::IsNumber("12312312313.23424523"), false);
		TEST_ASSERT_EQUALS(Strings::IsNumber("12312312313"), true);
		TEST_ASSERT_EQUALS(Strings::IsNumber("12312312313f"), false); // character at end
		TEST_ASSERT_EQUALS(Strings::IsNumber("18446744073709551616"), true); // 64
		TEST_ASSERT_EQUALS(Strings::IsNumber("-18"), true);
		TEST_ASSERT_EQUALS(Strings::IsNumber("-f18"), false);
		TEST_ASSERT_EQUALS(Strings::IsNumber("-18446744073709551616"), true); // 64
	}

Test Results

eqemu@15bfea186d05:~/server$ server && ~/code/build/bin/tests
Import |    Info    | LoadPaths server [/home/eqemu/server] 
Import |    Info    | LoadPaths logs path [logs] 
Import |    Info    | LoadPaths lua mods path [] 
Import |    Info    | LoadPaths lua_modules path [quests/lua_modules] 
Import |    Info    | LoadPaths maps path [maps] 
Import |    Info    | LoadPaths patches path [assets/patches] 
Import |    Info    | LoadPaths plugins path [quests/plugins] 
Import |    Info    | LoadPaths quests path [quests] 
Import |    Info    | LoadPaths shared_memory path [shared] 
MemoryMappedFileTest: 2/2, 100% correct in 0.000000 seconds
IPCMutexTest: 4/4, 100% correct in 0.000000 seconds
FixedMemoryHashTest: 12/12, 100% correct in 0.000000 seconds
FixedMemoryVariableHashTest: 8/8, 100% correct in 0.000000 seconds
atoboolTest: 13/13, 100% correct in 0.000000 seconds
hextoi_32_64_Test: 12/12, 100% correct in 0.000000 seconds
StringUtilTest: 7/7, 100% correct in 0.000000 seconds
DataVerificationTest: 4/4, 100% correct in 0.000000 seconds
SkillsUtilsTest: 2/2, 100% correct in 0.000000 seconds
TaskStateTest: 16/16, 100% correct in 0.000000 seconds
Total: 80 tests, 100% correct in 0.000000 seconds

@Akkadius Akkadius merged commit 2a6cf8c into master Mar 4, 2023
@Akkadius Akkadius deleted the feature/strings_methods branch March 4, 2023 23:01
nytmyr pushed a commit to nytmyr/Server that referenced this pull request Dec 1, 2023
* [Strings] Add more number formatters

- Adds `Strings::ToUnsignedInt` for `uint32` support.
- Adds `Strings::ToBigInt` for `int64` support.
- Adds `Strings::ToUnsignedBigInt` for `uint64` support.
- Adds `Strings::ToFloat` for `float` support.
- Replaces all `std::stoi` references with `Strings::ToInt`.
- Replaces all `atoi` references with `Strings::ToInt`.
- Replaces all `std::stoul` references with `Strings::ToUnsignedInt`.
- Replaces all `atoul` references with `Strings::ToUnsignedInt`.
- Replaces all `std::stoll` references with `Strings::ToBigInt`.
- Replaces all `atoll` references with `Strings::ToBigInt`.
- Replaces all `std::stoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `atoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `std::stof` references with `Strings::ToFloat`.

* [Strings] Add more number formatters

- Adds `Strings::ToUnsignedInt` for `uint32` support.
- Adds `Strings::ToBigInt` for `int64` support.
- Adds `Strings::ToUnsignedBigInt` for `uint64` support.
- Adds `Strings::ToFloat` for `float` support.
- Replaces all `std::stoi` references with `Strings::ToInt`.
- Replaces all `atoi` references with `Strings::ToInt`.
- Replaces all `std::stoul` references with `Strings::ToUnsignedInt`.
- Replaces all `atoul` references with `Strings::ToUnsignedInt`.
- Replaces all `std::stoll` references with `Strings::ToBigInt`.
- Replaces all `atoll` references with `Strings::ToBigInt`.
- Replaces all `std::stoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `atoull` references with `Strings::ToUnsignedBigInt`.
- Replaces all `std::stof` references with `Strings::ToFloat`.

* Rebase cleanup

* Changes/benchmarks/tests

---------

Co-authored-by: Akkadius <akkadius1@gmail.com>
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.

3 participants