Skip to content

Comments

Use assert instead of enforce in std.numeric#4688

Merged
9il merged 1 commit intodlang:masterfrom
PetarKirov:use-assert-in-std-numeric
Aug 7, 2016
Merged

Use assert instead of enforce in std.numeric#4688
9il merged 1 commit intodlang:masterfrom
PetarKirov:use-assert-in-std-numeric

Conversation

@PetarKirov
Copy link
Member

... in order to make the code more @nogc and nothrow friendly.

And also use onOutOfMemoryError instead of allocating and throwing a new
exception when malloc fails.

std/numeric.d Outdated
// No infinity support?
enforce(sig != 0, "Infinity floating point value assigned to a "
~ typeof(this).stringof~" (no infinity support).");
assert (sig != 0, "Infinity floating point value assigned to a "
Copy link
Contributor

Choose a reason for hiding this comment

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

No space after assert. Same throughout

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in the style guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

assert() is by fair the dominant style in Phobos, and matches function calls. Please change it. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

and matches function calls

Which is why I am against it. But let's agree to disagree for now.

@JackStouffer
Copy link
Contributor

Technically this is a breaking change. I have no opinion on if this should be merged or not but this should get approval from many core team members before getting merged.

@PetarKirov PetarKirov force-pushed the use-assert-in-std-numeric branch from 4744a41 to c460851 Compare July 29, 2016 14:56
@PetarKirov
Copy link
Member Author

All of the changes were cases of irrecoverable errors. enforce should have never been used for them.

@9il 9il added the math label Jul 29, 2016
@codecov-io
Copy link

codecov-io commented Jul 29, 2016

Current coverage is 88.68% (diff: 88.88%)

Merging #4688 into master will increase coverage by <.01%

@@             master      #4688   diff @@
==========================================
  Files           121        121          
  Lines         73850      73852     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          65494      65497     +3   
+ Misses         8356       8355     -1   
  Partials          0          0          

Powered by Codecov. Last update 79d1195...a724972

@dnadlinger
Copy link
Contributor

All of the changes were cases of irrecoverable errors. enforce should have never been used for them.

That's not true. For example, the enforce in euclideanDistance it's very much a question of API design. Sure, it would be reasonable to declare inputs with mismatching lengths a logic error and just assert on them. But throwing is equally as valid a design decision if you want to accept arbitrary arrays, even if the former might generally preferred in our circles.

@PetarKirov
Copy link
Member Author

I would argue that any code that tries to calculate euclideanDistance of two non equal length ranges has a serious problem.

@dnadlinger
Copy link
Contributor

That might be a valid argument (or might not be, if the vectors come from untrusted input), but it irrelevant here either case. This is a breaking API change, and we need to treat it as such.

@PetarKirov
Copy link
Member Author

AFAIR, there were other PRs that changed enforces to asserts inside .popFront for empty ranges and they were accepted. To me the situation is analogous.

This is a breaking API change, and we need to treat it as such.

I seriously doubt anyone is relying on being able to catch bare Exceptions from std.numeric, but anyway, how would you propose to mitigate the breakage?

@dnadlinger
Copy link
Contributor

You might have a point in so far as the euclideanDistance documentation actually says "The two ranges must have the same length."

I'll leave this to others to decide.

@PetarKirov PetarKirov force-pushed the use-assert-in-std-numeric branch from c460851 to 9a2fde8 Compare July 29, 2016 19:05
@9il
Copy link
Member

9il commented Jul 30, 2016

LGTM

Will wait for yet another one LGTM from core team before merge

@wilzbach
Copy link
Contributor

For what it is worth needing to catch exceptions in math code makes it slower and annoying to use.
So I like this move, my code wants to be pure nothrow @safe @nogc ;-)
Thanks @ZombineDev

@PetarKirov
Copy link
Member Author

@wilzbach I will make a pass over the module and add pure nothrow @safe @nogc to the unittests, but I first need this PR merged.

@DmitryOlshansky
Copy link
Member

These look more like asserts then enforces to me.

@JackStouffer
Copy link
Contributor

@ZombineDev Please add a warning in the changelog about this change

@PetarKirov
Copy link
Member Author

Alright, will do later today.

@PetarKirov PetarKirov force-pushed the use-assert-in-std-numeric branch from 9a2fde8 to 54652b3 Compare August 3, 2016 22:19
And also use onOutOfMemoryError instead of allocating and throwing a new
exception when malloc fails.
@PetarKirov PetarKirov force-pushed the use-assert-in-std-numeric branch from 54652b3 to d9abb10 Compare August 3, 2016 22:28
@PetarKirov
Copy link
Member Author

I added a changelog entry. Let me know if there's anything else I need to do.

@PetarKirov
Copy link
Member Author

Is there a way to see the rendered changelog on @CyberShadow's tester?

@PetarKirov PetarKirov closed this Aug 3, 2016
@PetarKirov PetarKirov reopened this Aug 3, 2016
@wilzbach
Copy link
Contributor

wilzbach commented Aug 3, 2016

Is there a way to see the rendered changelog on @CyberShadow's tester?

No sadly not atm - see #4228 for details, for now you can only compile the file on its own with e.g. dmd -D changelog.dd.

@PetarKirov
Copy link
Member Author

Ping @9il @DmitryOlshansky, is there anything else for me to do?

@9il
Copy link
Member

9il commented Aug 7, 2016

Auto-merge toggled on

@9il 9il merged commit 4a634ed into dlang:master Aug 7, 2016
@PetarKirov
Copy link
Member Author

Thanks, @9il!

@PetarKirov PetarKirov deleted the use-assert-in-std-numeric branch August 7, 2016 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants