refactor: Change TryParse implementation #1442

jadebenn · 2024-02-02T23:53:30Z

Changed the TryParse function to use from_chars(), natively handle enums, and have a better syntax. Also did some cleanup along the way, particularly regarding the function declarations of QuickbuildComponent.h.

Need to see how these changes compile on the other platforms, but have had good experience testing on my own.

…eanup along the way)

EmosewaMC · 2024-02-03T00:39:21Z

Can you elaborate on general cleanup?

jadebenn · 2024-02-03T00:50:33Z

Can you elaborate on general cleanup?

I can try and split out the unrelated changes later. It's kind of hard because I tend to notice and address issues as I'm working on a primary goal, but it isn't good practice to mix them up I agree.

For the most part, they're really minor. A few [[nodiscard]] tags here, a few consts there... that sort of thing.

EmosewaMC · 2024-02-03T00:56:52Z

For the most part, they're really minor. A few [[nodiscard]] tags here, a few consts there... that sort of thing.

Just as a note, we wont be enforcing nodiscard as an attribute in any prs, nor will we be requiring it for apis. Nothing against adding them, just wont be enforcing it

jadebenn · 2024-02-03T03:29:18Z

For the most part, they're really minor. A few [[nodiscard]] tags here, a few consts there... that sort of thing.

Just as a note, we wont be enforcing nodiscard as an attribute in any prs, nor will we be requiring it for apis. Nothing against adding them, just wont be enforcing it

Didn't expect any different, just trying to do my part for keeping code intentions clear.

As far as the failed CI, I suspected that it wouldn't work on MacOS given our previous conversations, so I actually have the fallback floating point code ready to go (just commented out). I much prefer from_chars though, so I'll probably wrap it in a pre-processor block so it is ONLY present on Mac.

One neat thing is that the way the concept resolution works, I don't actually need to change anything in the other templates to make floating points switch over: They see the more specific constraint and immediately start using that one. At least, this is the behavior I saw on my end.

EmosewaMC · 2024-02-03T05:16:47Z

how has this been tested?

EmosewaMC

I am only reviewing the GeneralUtils.h header for now until the other unrelated changes are reverted. There are a lot of unrelated changes here which make the pr much larger to review than it needs to be

dCommon/GeneralUtils.h

jadebenn · 2024-02-03T06:46:24Z

how has this been tested?

I've run around VE, AG, and Ninjago completing missions and playing normally for about 90 minutes and haven't noticed anything strange with the Ubuntu build.

jadebenn · 2024-02-03T07:08:24Z

I am only reviewing the GeneralUtils.h header for now until the other unrelated changes are reverted. There are a lot of unrelated changes here which make the pr much larger to review than it needs to be

I don't think there were very many? I had to do a lot of changes relating to the actual TryParse invocations, but aside from QuickbuildComponent (which I will split out sometime tomorrow) it was pretty much just tossing a few 'consts' on things I saw that didn't need to be mutable. Even QuickbuildComponent was just adjusting for const correctness, attrib tagging, and marking noexcept.

jadebenn · 2024-02-03T07:10:53Z

Ah, forgot about dCommonVars.h. I see what you mean a bit better now.

EmosewaMC · 2024-02-03T07:12:16Z

I am only reviewing the GeneralUtils.h header for now until the other unrelated changes are reverted. There are a lot of unrelated changes here which make the pr much larger to review than it needs to be

I don't think there were very many? I had to do a lot of changes relating to the actual TryParse invocations, but aside from QuickbuildComponent (which I will split out sometime tomorrow) it was pretty much just tossing a few 'consts' on things I saw that didn't need to be mutable. Even QuickbuildComponent was just adjusting for const correctness, attrib tagging, and marking noexcept.

In the future we'd appreciate isolated pr changes for things that are very much not related.

EmosewaMC · 2024-02-03T07:13:29Z

Yeah commonvars.h is the main one I was looking at. With those being really compiler only changes they would be better suited in their own pr, but if you don't want to move them now it's fine, just will take more time to review

dCommon/GeneralUtils.h

dGame/dComponents/TriggerComponent.cpp

dGame/dInventory/Item.cpp

dGame/dUtilities/SlashCommandHandler.cpp

dMasterServer/InstanceManager.cpp

dNet/AuthPackets.cpp

dScripts/02_server/Map/AM/AmSkullkinTower.cpp

dZoneManager/Level.cpp

jadebenn · 2024-02-04T04:23:03Z

Actually, no I'm not going to rename that variable with the m_ prefix. That would need to be a PR of its own because it creates so many problems. I will be reverting my commits to that effect.

This reverts commit bf318ca.

This reverts commit bf318ca. Fully revert renaming attempt

…meServer into TryParseChanges

dCommon/dEnums/dCommonVars.h

…meServer into TryParseChanges

…THIS should be the last commit pending CI

… they're C++ headers. Pinky swear this is it!

…meServer into TryParseChanges

jadebenn · 2024-02-04T18:26:31Z

okay pinky swear that is It. I will resist the urge to add anything more unless the CI fails again

dNet/AuthPackets.cpp

dGame/dUtilities/SlashCommandHandler.cpp

EmosewaMC

pending ci

* Changed how the TryParse function works (and also did some general cleanup along the way) * Update noexcept attributes (verified these are correct) * Add fp overload for MacOS functionality * resolving some feedback * Split out unrelated changes to CleanupRoundup branch * Update in response to feedback * the consequences of emo's member variable renaming request * Revert "the consequences of emo's member variable renaming request" This reverts commit bf318ca. * Fully revert renaming attempt * Revert "the consequences of emo's member variable renaming request" This reverts commit bf318ca. Fully revert renaming attempt * Created ClientVersion.h and moved the client version defaults to it * Fix partial parsing and MacOS floating point errors * attempting fix to MacOS compiler error * syntax pass (should be the last commit unless the CI fails) * ah, wait, forgot to uncomment the preprocessor statements for MacOS. THIS should be the last commit pending CI * Okay, one last thing I noticed: We were including C headers here. Now they're C++ headers. Pinky swear this is it! * typo and I am OCD. please let this be the last * hash is usally but not always noexcept, so the specifier should go * Address MOST of the feedback * address the claim codes issue

jadebenn and others added 3 commits February 2, 2024 17:50

Changed how the TryParse function works (and also did some general cl…

4f75479

…eanup along the way)

Merge branch 'DarkflameUniverse:main' into TryParseChanges

bc26988

Update noexcept attributes (verified these are correct)

bea9c38

jadebenn marked this pull request as ready for review February 3, 2024 00:07

Add fp overload for MacOS functionality

e0e686f

EmosewaMC requested changes Feb 3, 2024

View reviewed changes

resolving some feedback

b3dd1d3

Split out unrelated changes to CleanupRoundup branch

74d8a5b

jadebenn changed the title ~~chore: Change how the TryParse function works (and also some general cleanup)~~ chore: Change how the TryParse function works Feb 3, 2024

jadebenn changed the title ~~chore: Change how the TryParse function works~~ refactor: Change how the TryParse function works Feb 3, 2024

jadebenn mentioned this pull request Feb 3, 2024

chore: General cleanup roundup #1444

Merged

jadebenn changed the title ~~refactor: Change how the TryParse function works~~ refactor: Change TryParse implementation Feb 3, 2024

EmosewaMC reviewed Feb 3, 2024

View reviewed changes

dCommon/GeneralUtils.h Outdated Show resolved Hide resolved

EmosewaMC reviewed Feb 3, 2024

View reviewed changes

Update in response to feedback

27f69d6

jadebenn requested a review from EmosewaMC February 3, 2024 23:52

the consequences of emo's member variable renaming request

bf318ca

jadebenn added 2 commits February 3, 2024 22:26

Revert "the consequences of emo's member variable renaming request"

1dc836d

This reverts commit bf318ca.

Fully revert renaming attempt

86b954b

jadebenn and others added 2 commits February 3, 2024 22:35

Revert "the consequences of emo's member variable renaming request"

2d8f282

This reverts commit bf318ca. Fully revert renaming attempt

Merge branch 'TryParseChanges' of https://github.com/jadebenn/Darkfla…

926a2de

…meServer into TryParseChanges

EmosewaMC reviewed Feb 4, 2024

View reviewed changes

dCommon/dEnums/dCommonVars.h Outdated Show resolved Hide resolved

jadebenn and others added 7 commits February 3, 2024 23:11

Created ClientVersion.h and moved the client version defaults to it

f52af77

Merge branch 'TryParseChanges' of https://github.com/jadebenn/Darkfla…

e5d67e7

…meServer into TryParseChanges

Fix partial parsing and MacOS floating point errors

b9e46a1

attempting fix to MacOS compiler error

3a7b4ed

syntax pass (should be the last commit unless the CI fails)

b3aa890

ah, wait, forgot to uncomment the preprocessor statements for MacOS. …

61e8e0f

…THIS should be the last commit pending CI

Okay, one last thing I noticed: We were including C headers here. Now…

b1df63e

… they're C++ headers. Pinky swear this is it!

jadebenn requested a review from EmosewaMC February 4, 2024 18:15

jadebenn and others added 3 commits February 4, 2024 12:17

typo and I am OCD. please let this be the last

116ec63

hash is usally but not always noexcept, so the specifier should go

0ab30c7

Merge branch 'TryParseChanges' of https://github.com/jadebenn/Darkfla…

0c6d025

…meServer into TryParseChanges

EmosewaMC requested changes Feb 5, 2024

View reviewed changes

jadebenn and others added 4 commits February 8, 2024 19:39

Merge branch 'DarkflameUniverse:main' into TryParseChanges

6cb3123

Address MOST of the feedback

7c2a754

Merge branch 'DarkflameUniverse:main' into TryParseChanges

79a8bac

address the claim codes issue

0bc2b8a

jadebenn requested review from aronwk-aaron and EmosewaMC February 10, 2024 05:51

EmosewaMC approved these changes Feb 10, 2024

View reviewed changes

aronwk-aaron approved these changes Feb 10, 2024

View reviewed changes

aronwk-aaron merged commit 0c1ee05 into DarkflameUniverse:main Feb 10, 2024
4 checks passed

jadebenn deleted the TryParseChanges branch February 10, 2024 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Change TryParse implementation #1442

refactor: Change TryParse implementation #1442

jadebenn commented Feb 2, 2024 •

edited

EmosewaMC commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024

EmosewaMC left a comment •

edited

jadebenn commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024 •

edited

EmosewaMC commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024

jadebenn commented Feb 4, 2024

jadebenn commented Feb 4, 2024

EmosewaMC left a comment

refactor: Change TryParse implementation #1442

refactor: Change TryParse implementation #1442

Conversation

jadebenn commented Feb 2, 2024 • edited

EmosewaMC commented Feb 3, 2024 • edited

jadebenn commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024 • edited

jadebenn commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024

EmosewaMC left a comment • edited

Choose a reason for hiding this comment

jadebenn commented Feb 3, 2024 • edited

jadebenn commented Feb 3, 2024 • edited

jadebenn commented Feb 3, 2024 • edited

EmosewaMC commented Feb 3, 2024

EmosewaMC commented Feb 3, 2024

jadebenn commented Feb 4, 2024

jadebenn commented Feb 4, 2024

EmosewaMC left a comment

Choose a reason for hiding this comment

jadebenn commented Feb 2, 2024 •

edited

EmosewaMC commented Feb 3, 2024 •

edited

EmosewaMC commented Feb 3, 2024 •

edited

EmosewaMC left a comment •

edited

jadebenn commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024 •

edited

jadebenn commented Feb 3, 2024 •

edited