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

Revert "Add integral overload for isNaN for use in generic code" #2366

Merged
merged 1 commit into from
Jul 25, 2014

Conversation

WalterBright
Copy link
Member

Reverts #2311

I should have looked at this sooner.

This is a terrible idea (!). To quote Don Clugston "I think it's a complete fantasy to think you can write generic code that will work for both floats and ints. The algorithms are completely different. [...] Producing generic code that works for both floats and ints is a fool's errand."

Once you start going down this path,

1.where do you stop? What about isInfinity?

2.it'll never work. Don is right.

@mihails-strasuns
Copy link

1.where do you stop? What about isInfinity?

What is the problem with supporting every such template? What is the harm?

@WalterBright
Copy link
Member Author

Don's quote amply signifies the problem and the harm. Floating point and integral arithmetic are fundamentally different for every single operation on them. If you write generic math code and expect it to work with integral and float arithmetic you are in for a world of frustration and bugs. We should not be pretending to support that. Here it is in context:

http://forum.dlang.org/post/pdebgkcijzijoeijmlsm@forum.dlang.org

@WalterBright
Copy link
Member Author

What is the harm?

"What is it good for" is a much more apropos question. A library should have as few features as practical to cover the problem space, not as many features as don't interfere with getting problems solved.

@mihails-strasuns
Copy link

No I don't expect any floating operations to actually work on integer - for me this is in the same domain as is(T == SomeType) check. Most likely user code that will do the same "go with the false branch" anyway.

Anyway, to avoid any further misunderstanding - does that mean that in general that calling any traits on inapplicable types should result in an error instead of false value? Or is it only about floating points?

@Safety0ff
Copy link
Contributor

We should not forget about the context of the original pull: http://forum.dlang.org/thread/cvhbtzspttcvlelnyeoe@forum.dlang.org

@Hackerpilot
Copy link
Member

Yeah. It's funny how some breaking changes are fine and others aren't.

@WalterBright
Copy link
Member Author

@Safety0ff yes, thanks for the context. I'd say the original code may have worked, but was wrong.

Anyway, to avoid any further misunderstanding - does that mean that in general that calling any traits on inapplicable types should result in an error instead of false value?

Yes.

Or is it only about floating points?

I think it's especially true for FP, but in general it is a good practice. It flags at compile time code that is potentially buggy instead of having it go and do something undocumented at runtime. We should also, of course, exercise good judgment for each case rather than blind adherence to rules.

@WalterBright
Copy link
Member Author

It's funny how some breaking changes are fine and others aren't.

Blind adherence to principle as a substitute for good judgment is not a good idea. In this case, the code that used isnan for integers is something that looks an awful lot like a latent bug and should never have compiled.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 24, 2014

Blind adherence to principle as a substitute for good judgment is not a good idea. In this case, the code that used isnan for integers is something that looks an awful lot like a latent bug and should never have compiled.

FYI, I didn't like the idea either. But code that got unexpectedly broken is code that got unexpectedly broken...

f = cast(T)53.6;
assert(!isNaN(f));
assert(!isNaN(-f));
}
Copy link
Member

Choose a reason for hiding this comment

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

We can keep this TypeTuple magic perhaps.

@mihails-strasuns
Copy link

Blind adherence to principle as a substitute for good judgment is not a good idea

I am very tempted to agree but in practice that means that all declared goals of stability and backwards compatibility are a joke. That anything can break if it falls into domain of your good judgement - something no one from outside can guess.

How about at least keeping that overload but immediately marking it deprecated and getting rid of it again later?

@yebblies
Copy link
Member

How about at least keeping that overload but immediately marking it deprecated and getting rid of it again later?

This seems like a sane compromise.

@quickfur
Copy link
Member

What about the other std.math functions? Surely the templatization of these functions in 2.066 probably will break other existing code as well? So should we introduce deprecated int functions for all of them? That sounds a bit extreme.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 24, 2014

What about the other std.math functions? Surely the templatization of these functions in 2.066 probably will break other existing code as well?

If someone is passing an integer to isSubNormal, their program is broken anyway.

@mihails-strasuns
Copy link

Yes but it should gracefully notify user of that, not break suddenly. In my opinion every single function that accepted integers before templatization must have a deprecated overload for convenient transition.

@quickfur
Copy link
Member

OK, so we should have:

deprecated("Passing int to isSubNormal is deprecated")
int isSubNormal(T : int)(T t) {
    static assert(0, "Your program is broken");
}

or something like that? ;-)

@mihails-strasuns
Copy link

It should do the same broken thing as it did as built-in. But deprecated("Your program is broken" looks good :)

@WalterBright
Copy link
Member Author

I am very tempted to agree but in practice that means that all declared goals of stability and backwards compatibility are a joke. That anything can break if it falls into domain of your good judgement - something no one from outside can guess.

  1. For an analogy, consider a judge in a legal case. There's a reason we have judges, and not computer programs, dispensing justice. There are no perfectly written laws, and there are plenty of cases which are technically illegal but have circumstances which make it unjust to apply the law. (Yes, I often think judges make unjust decisions, too.)
  2. D's principles often conflict with each other. This is one such case.
  3. Saying that sometimes we inject human judgment into the process does not make principles a joke - that just goes hard over in the other direction.
  4. We do have a history of breaking the compilation of invalid code. As I posted earlier, I am not convinced that the code that successfully compiled before was correct code. Using isNan on integer data is something that would raise a large red flag on any code I was responsible for.

How about at least keeping that overload but immediately marking it deprecated and getting rid of it again later?

If this is a more pervasive issue than one instance, we can consider it.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 24, 2014

@WalterBright - are you going to fix-up the revert of the unittest?

@WalterBright
Copy link
Member Author

@ibuclaw I can do that if you like, but I'd rather do it separately rather than confuse this revert.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 25, 2014

Fair enough. Reverting as

  1. I'm the author of change
  2. I'm not compelled to do anything else given other comments or suggestions.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 25, 2014

Auto-merge toggled on

@ibuclaw
Copy link
Member

ibuclaw commented Jul 25, 2014

By the way @WalterBright, you seem to have created this branch on the main Phobos repository, rather than your own fork. I do believe this is discouraged.

ibuclaw added a commit that referenced this pull request Jul 25, 2014
Revert "Add integral overload for isNaN for use in generic code"
@ibuclaw ibuclaw merged commit 5062c55 into master Jul 25, 2014
@WalterBright
Copy link
Member Author

@ibuclaw I pushed the [Revert] button, a new github feature, and it did what it did :-)

@ibuclaw
Copy link
Member

ibuclaw commented Jul 25, 2014

New features always break something... :)
** deletes branch **

@ibuclaw ibuclaw deleted the revert-2311-intnan branch July 25, 2014 10:25
@ibuclaw
Copy link
Member

ibuclaw commented Jul 25, 2014

@9rnsr @AndrewEdwards Please merge this into 2.066

@dnadlinger
Copy link
Member

I do believe a stray branch is much less of a problem than a botched revert, though ;)

@mihails-strasuns
Copy link

@WalterBright

I simply can't understand how you can make statement like this and post comments like http://forum.dlang.org/post/lquo91$1616$1@digitalmars.com at the same time. Something is fundamentally broken in you understanding of breaking changes - "if it breaks code but looks right to me it does not really break the code". No wonder people usually giggle when hear things like "stability is priority goal for D".

People don't expect experience similar to court trial from compiler update. Quite the contrary, they expect anything but "trial" - reliable process with clear expectation that shows some respect to their problems. And given how opinionated language design is in general that means stupid formal processes hardly suitable in courts. That means providing migration paths even for code you 100% believe is broken. Likely so it is but it somehow works in their current project and telling them "it was broken anyway" is anything but professional attitude.

Now most of long-standing D users got used to the fact that stuff may break and adjusted their development process to take that into consideration - our crowd tends to be much more progress-happy than common corporate types that are usually most concerned about stability. Progress is necessary if language doesn't want to become another C++ incarnation - even Scott Meyers has made that point despite not being deeply familiar with our community. Sometimes you need to break stuff to fix things and we are happy to clean broken code.

But you can't pretend both! This issue was considered worth breaking by you, at the same catch () syntax was not, despite the fact it can result in real program corrupting errors. No one can guess what will be considered important enough and what won't. In the end for the user it means worst of both cases - any code can break unexpectedly but there is no hope for fixing any design issues. This is D development process disaster number one.

Pick whatever change strategy you consider wise but, please, let it be something other than "try guessing what I have in mind". Something that won't come as release surprise even if tough decision is to be made. Something that D programmers actually can rely on. This is what formal rules are for.

@WalterBright
Copy link
Member Author

I simply can't understand

It appears you are looking for an absolutist statement from me, or at least trying to impute one. I've explained over and over what the role of judgment is in making decisions. There is NO SUCH THING as any absolute inviolable principle in programming language design. It's always about tradeoffs.

"if it breaks code but looks right to me it does not really break the code"

I never said such a thing. I've given rationales. You may not agree with the rationale, but please don't say I didn't give one.

This is what formal rules are for.

Again, you're looking for an absolute rule. That's never going to happen, even if I deferred every decision to your sole discretion.

Designing a PL is like designing a house. You want a bigger closet? Which room is going to then be correspondingly smaller? And so on for every single decision. Suppose you design an airplane. Do you want safety above everything else? Then your airplane will never move an inch, let alone fly.

Even C++ now and then gives up on backwards compatibility, even though it is an inviolable principle not to.

"try guessing what I have in mind"

Again, I gave detailed rationales for these decisions.

@mihails-strasuns
Copy link

I am not asking for formal rules to make language decisions. I am asking for ones to proceed with such decisions. For example "we always provide deprecation warnings for any breaking change, even if old behavior looks stupid/wrong/odd". You seem to imply that impossibility to define absolute rules for actual language judgement imply impossibility to define absolute rules for implementing such decisions - and this is proven to be wrong by many successful release/development process schemes.

For example in this PR I have proposed a compromise to keep an old behavior in a deprecated overload for one release. It is relatively simple solution that fits our usual process rules and reduces frustration factor from a change. However you have rejected it because "code is broken anyway" and this is something very different from approving change itself. This is the very point where you stop judging language and start judging users and that is rarely appreciated.

"code is broken anyway" can be decent rationale for breaking stuff. I doubt there is even theoretical case when it can be considered rationale for abandoning deprecation path for changed stuff.

@mihails-strasuns
Copy link

Also,

Again, I gave detailed rationales for these decisions.

People who participate in discussions do not need backwards compatibility guarantees pretty much by definition. You should care about those who never open newsgroup until release is out and their code breaks without explanation - no rationale or warning for them.

@WalterBright
Copy link
Member Author

I have proposed a compromise to keep an old behavior in a deprecated overload for one release.

That may very well be a good idea. Please propose as a PR.

@mihails-strasuns
Copy link

That is relieving : #2374

I hope to see a day where it will be absolutely mandatory step and not a "good idea" :(

@WalterBright
Copy link
Member Author

absolutely mandatory step

Really, we should not have absolutist rules.

AndrewEdwards added a commit that referenced this pull request Jul 27, 2014
Merge pull request #2366 from D-Programming-Language/revert-2311-intnan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants