Skip to content

Correctly handle duplicate WellKnownType values - #533

Merged
jaredpar merged 2 commits into
dotnet:masterfrom
jaredpar:small-fixes
Feb 17, 2015
Merged

Correctly handle duplicate WellKnownType values#533
jaredpar merged 2 commits into
dotnet:masterfrom
jaredpar:small-fixes

Conversation

@jaredpar

Copy link
Copy Markdown
Member

The WellKnownType enumeration contains several named items that have the
same underlying value. Both First and Last share a value with an actual
type entry in the enumeration. A ToString call on those values can
return either the type name or First / Last, the runtime does not define
which will happen.

This comes into play in the static constructor of WellKnownTypes where
we assert that the enumeration and s_metadataNames table stay in sync.
The code already accounts for First being ambiguous but failed to for
Last.

This change removes the assumption for Last as well. It also expands
out the assert as it was getting to difficult to follow when I added
another level of ternary expressions into the mix.

The WellKnownType enumeration contains several named items that have the
same underlying value.  Both First and Last share a value with an actual
type entry in the enumeration.  A ToString call on those values can
return either the type name or First / Last, the runtime does not define
which will happen.

This comes into play in the static constructor of WellKnownTypes where
we assert that the enumeration and s_metadataNames table stay in sync.
The code already accounts for First being ambiguous but failed to for
Last.

This change removes the assumption for Last as well.  It also expands
out the assert as it was getting to difficult to follow when I added
another level of ternary expressions into the mix.
@jaredpar

Copy link
Copy Markdown
Member Author

CC @gafter @AlekseyTs @agocke

@agocke

agocke commented Feb 16, 2015

Copy link
Copy Markdown
Member

I'm not a fan of ifdefs -- any specific reason you chose one instead of a helper method + debug assert call?

@jaredpar

Copy link
Copy Markdown
Member Author

@agocke because extending the existing Debug.Assert was unreadable. I'm fine with a separate method altogether.

@agocke

agocke commented Feb 16, 2015

Copy link
Copy Markdown
Member

@jaredpar I would prefer refactoring into a helper method, then calling that in Debug.Assert, if you have no objection.

@jaredpar

Copy link
Copy Markdown
Member Author

@agocke I was just going to have a [Conditional("DEBUG")] method that held the logic.

@agocke

agocke commented Feb 16, 2015

Copy link
Copy Markdown
Member

@jaredpar That works too. Anything that preserves semantic analysis in the compilation regardless of your defined variables :)

This refactors the code which asserts the enum and name table are in
sync into a new method.
@AlekseyTs

Copy link
Copy Markdown
Contributor

LGTM

jaredpar added a commit that referenced this pull request Feb 17, 2015
Correctly handle duplicate WellKnownType values
@jaredpar
jaredpar merged commit 5f40257 into dotnet:master Feb 17, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're going to turn one enum into a particular name, you should assert that they represent the same element, e.g.:

   Debug.Assert(WellKnownType.Last == WellKnownType.System_IFormatProvider);

@gafter

gafter commented Feb 17, 2015

Copy link
Copy Markdown
Member

👍

@agocke

agocke commented Feb 17, 2015

Copy link
Copy Markdown
Member

LGTM

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.

5 participants