Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix false contract assumption encountered on System.Decimal.op_Explicit #203

Merged

Conversation

yaakov-h
Copy link
Contributor

I don't think this has any actual effect on the release builds, but it stopped me getting an exception here when debugging.

op_Explicit, op_Implicit, and op_UnaryNegation all have just one argument, so the assumption that all operations for System.Decimal have two or more arguments is incorrect.

@SergeyTeplyakov
Copy link
Contributor

Two questions:

  1. Can we add Contract.Assume(args.Count >= 1); or Contract.Assume(args.Count == 1); for case "op_UnaryNegation":
  2. Should we use Contract.Assume(args.Count == 2); instead of Contract.Assume(args.Count >= 2);?

I'm not sure about second one, maybe it can break something, but first one seems very reasonable for me.

@yaakov-h
Copy link
Contributor Author

  1. Yes, I missed that one.
  2. I also thought it was odd we were assuming >= when it appears that == is always valid. I don't see what it could break or what could break it.

@SergeyTeplyakov
Copy link
Contributor

Could you please update the PR to address the first comment?
I think we can live with without fixing second one.

@SergeyTeplyakov SergeyTeplyakov added bug and removed bug labels Aug 11, 2015
@yaakov-h yaakov-h force-pushed the fix-system-decimal-assumption branch from b05a9b4 to 8ba03ed Compare August 11, 2015 23:09
@yaakov-h
Copy link
Contributor Author

Sorry about the delay, it's been nighttime here.

Updated.

SergeyTeplyakov added a commit that referenced this pull request Aug 12, 2015
Fix false contract assumption encountered on System.Decimal.op_Explicit
@SergeyTeplyakov SergeyTeplyakov merged commit 0dafe52 into microsoft:master Aug 12, 2015
@yaakov-h yaakov-h deleted the fix-system-decimal-assumption branch April 24, 2018 13:41
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.

2 participants