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

Fixed Some Array Extensions Bugs #157

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

martindevans
Copy link
Contributor

@martindevans martindevans commented Oct 26, 2023

I started testing the ArrayExtension methods and discovered a bug in the Add and GetOrResize methods. Both methods internally call Array.Resize, but they never return the new array! This is probably because the method is very badly named and implies that it will somehow resize an existing array, this is not the case. To quote the docs:

This method allocates a new array with the specified size, copies elements from the old array to the new one, and then replaces the old array with the new one.


The GetOrResize method was not used anywhere, so I simply deleted it.

The Add method was only used in the CompileTimeStatics class, which was using a Type[] as a map from int -> Type. Replaced this with an actual Dictionary<int, Type> instead, removing the need for the extension.

…Array.Resize` to resize the array, but never actually stores the re-sized array anywhere!

  - This method was only used in `CompileTimeStatics`, replace usage there with a `Dictionary<int, Type>` instead.
 - Removed Array `GetOrResize` extension method. Same reason as Add.
  - This method was not used anywhere.
@genaray
Copy link
Owner

genaray commented Oct 26, 2023

Thanks! Not on my PC right now, but having Arrays there ist intended due to the New lookup mechanism. Having dictionarys there ist extremely Bad for the lookups. So I would suggest to Just fix the Bug and leave the Arrays that way ^^

@martindevans
Copy link
Contributor Author

martindevans commented Oct 26, 2023

Ah I did have a look for "hot" paths using this (and didn't see any). If there's a new system on the way that uses this somewhere hot I'll have to move it back to arrays.

  - Added test coverage
 - Replaced `Dictionary` in `CompileTimeStatics` with an array, so that lookups are faster.
@martindevans
Copy link
Contributor Author

Ok I've added back in the Add extension, covered it with tests and used in inside ComponentRegistry.

Fixed:

  • Modified array is returned (same as the other Add extensions)
  • doublingCount is never allowed to be zero
  • Negative index is caught

Changed:

  • TypeToComponentType and Types now return readonly wrappers over mutable private fields, instead of publicly exposing mutable fields.

Copy link
Owner

@genaray genaray left a comment

Choose a reason for hiding this comment

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

Nice, great job! :)

@genaray genaray merged commit 353f7f6 into genaray:master Oct 27, 2023
1 check passed
@martindevans martindevans deleted the fix/array_extensions_add branch October 27, 2023 01:20
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