Skip to content

Fixes #3207. Prevent usage of KeyType without parameter. #3852

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

Merged
merged 1 commit into from
Jul 1, 2019
Merged

Fixes #3207. Prevent usage of KeyType without parameter. #3852

merged 1 commit into from
Jul 1, 2019

Conversation

harishsk
Copy link
Contributor

@harishsk harishsk commented Jun 11, 2019

Added an exception to the parameterless constructor for KeyType because it doesn't make sense to use it without a parameter

When KeyType is called without a constructor, the Count member remains uninitialized and when that remains null, it is set to uint.MaxValue which fails the overflow check.

Since the value that a particular KeyType takes on represents an index into a 1 based vector, it cannot be set to zero by zero by default. It always needs to have a valid max value and that needs to be determined by the use case. Therefore, the best approach is to prevents its usage without the parameter.

There seems to be a different but related doc bug that this fix does not address:

The documentation for KeyTypeAttribute says it can be used with uint and ulong. However, during mapping there is an overflow check performed and an exception is thrown if the value is greater than int.MaxValue - 1.

That means, we don't actually support the full range of uint and ulong.

@codemzs / @artidoro Any thoughts on whether we should fix the docs or remove the overflow check?

  • There's a descriptive title that will make sense to other developers some time from now.
  • There's associated issues. All PR's should have issue(s) associated - unless a trivial self-evident change such as fixing a typo. You can use the format Fixes #nnnn in your description to cause GitHub to automatically close the issue(s) when your PR is merged.
  • Your change description explains what the change does, why you chose your approach, and anything else that reviewers should know.
  • You have included any necessary tests in the same PR.

fixes #3207

…KeyType because it doesn't make sense to use it without a parameter
@codemzs codemzs requested review from codemzs, wschin and artidoro June 27, 2019 17:03
@codemzs codemzs merged commit a1ede65 into dotnet:master Jul 1, 2019
Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
…r for KeyType because it doesn't make sense to use it without a parameter (dotnet#3852)
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error for KeyType attribute without initializing the Count
2 participants