Add predefined version identifier `D_BoundsChecks` by Geod24 · Pull Request #8532 · dlang/dmd

Geod24 · 2018-08-02T08:43:46Z

One stylistic rule that is strictly enforced in D is that
version identifier and CLI switch should be positive.
`D_NoBoundsChecks` stands out as an exception in this regard.
This PR just adds `D_BoundsChecks` so that code have at least
one release to transition to it, before `D_NoBoundsChecks` is deprecated.

dlang-bot · 2018-08-02T08:43:46Z

Thanks for your pull request and interest in making D better, @Geod24! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8532"

JinShil · 2018-08-02T11:38:22Z

changelog/d_boundscheck.dd

+
+One stylistic rule of D is that version identifier should be positive.
+`D_NoBoundsChecks` stands out as an exception in this regard.
+This version adds the opposite `D_BoundsChecks` so that code have at least


I think we might be subject to the deprecation policy being proposed at https://github.com/dlang/DIPs/blob/master/DIPs/DIP1013.md

JinShil · 2018-08-02T11:39:06Z

src/dmd/mars.d

        VersionCondition.addPredefinedGlobalIdent("assert");
    if (global.params.useArrayBounds == CHECKENABLE.off)
        VersionCondition.addPredefinedGlobalIdent("D_NoBoundsChecks");
+    else // Introduced in 2.082.0 - Deprecate `D_NoBoundsChecks` later


Use the deprecation comment syntax proposed in https://github.com/dlang/DIPs/blob/master/DIPs/DIP1013.md for easy grepping, and remain consistence with other deprecations in the code base.

JinShil · 2018-08-02T11:40:33Z

Fine with me after we have a spec update and we get deprecation policy sorted out.

WalterBright

It's not "strictly" enforced, just a strong bias that direction. Also, it should be removed from the documentation (and this added).

Geod24 · 2018-08-03T01:26:27Z

@JinShil : Thanks for the link, didn't knew about it.
I wanted to introduce the version identifier for at least one release before deprecating the other identifier, so that transition is smoother. What do you think ?

As explained in dlang/dmd#8532 the prefered style is positive version identifiers. The DMD PR adds the new version identifier to be used for at least a release before D_NoBoundsChecks is deprecated. In the meantime, it can already be undocumented.

Geod24 · 2018-08-03T01:48:55Z

dlang/dlang.org#2438

Geod24 · 2018-08-03T04:32:25Z

Also found https://issues.dlang.org/show_bug.cgi?id=19137 out

wilzbach · 2018-08-03T11:28:36Z

changelog/d_boundscheck.dd

+
+One stylistic rule of D is that version identifier should be positive.
+`D_NoBoundsChecks` stands out as an exception in this regard.
+This version adds the opposite `D_BoundsChecks` so that code have at least


I think it should be either code bases or projects (code can't transition itself, can it?)

WalterBright · 2018-08-04T00:00:57Z

Also found https://issues.dlang.org/show_bug.cgi?id=19137 out

Good catch. I withdraw my approval until this is sorted out.

WalterBright

Need to resolve bug 19137

Geod24 · 2018-08-06T02:43:52Z

@WalterBright : Could you give your opinion on said bug ? My original thought was that we could apply D_BoundsChecks when the scope is @safe but I'm afraid it will introduce a bad precedent. E.g. what about a template specialization which is @safe and has invalid code in its D_BoundsChecks versioned code ?

On the other hand @JinShil proposed to introduce D_SafeBoundsChecks, which is a much simpler approach but makes it more difficult for the user. One has to know which one to use, and it might not be possible (e.g. when templates are used).

Do you have an alternative idea, or do you prefer one of those ?

One stylistic rule that is strictly enforced in D is that version identifier and CLI switch should be positive. `D_NoBoundsChecks` stands out as an exception in this regard. This PR just adds `D_BoundsChecks` so that code have at least one release to transition to it, before `D_NoBoundsChecks` is deprecated.

As explained in dlang/dmd#8532 the prefered style is positive version identifiers. The DMD PR adds the new version identifier to be used for at least a release before D_NoBoundsChecks is deprecated. In the meantime, it can already be undocumented.

ghost

This not worth starting a deprecation. New version identifiers however should follow the rule of being "positive". Also even if unlikey, the new reserved version could exist somewhere in some code, so a deprecation would also be required, making this (even if simple) change a long process.

I suggest @Geod24 to close.

Geod24 · 2020-08-03T11:25:54Z

Yes the benefit of this is very low, hence why I haven't pursued it. However, one note: version identifiers starting with D_ are reserved so there wouldn't be a need for a deprecation.

ghost · 2020-08-03T11:27:21Z

ok, note taked.

Geod24 added Review:Easy Review Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Aug 2, 2018

Geod24 requested review from MartinNowak, WalterBright and rainers as code owners August 2, 2018 08:43

JinShil reviewed Aug 2, 2018

View reviewed changes

WalterBright approved these changes Aug 2, 2018

View reviewed changes

Geod24 mentioned this pull request Aug 3, 2018

Un-document D_NoBoundsChecks and replace it with D_BoundsChecks dlang/dlang.org#2438

Closed

JinShil approved these changes Aug 3, 2018

View reviewed changes

Geod24 closed this Aug 3, 2018

Geod24 reopened this Aug 3, 2018

wilzbach reviewed Aug 3, 2018

View reviewed changes

WalterBright requested changes Aug 4, 2018

View reviewed changes

JinShil mentioned this pull request Aug 7, 2018

Convert _d_arraycast to template dlang/druntime#2264

Merged

Geod24 removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Aug 7, 2018

Geod24 force-pushed the no-no-yes branch from ec5b9e9 to b700463 Compare September 27, 2018 05:35

thewilsonator self-assigned this Nov 7, 2018

ghost suggested changes Aug 3, 2020

View reviewed changes

ghost added the Review:Needs Rebase label Aug 3, 2020

Geod24 closed this Aug 3, 2020

Uh oh!

Conversation

Geod24 commented Aug 2, 2018

Uh oh!

dlang-bot commented Aug 2, 2018

Bugzilla references

Testing this PR locally

Uh oh!

JinShil Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil Aug 2, 2018

Choose a reason for hiding this comment

Uh oh!

JinShil commented Aug 2, 2018

Uh oh!

WalterBright left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 3, 2018

Uh oh!

Geod24 commented Aug 3, 2018

Uh oh!

Geod24 commented Aug 3, 2018

Uh oh!

wilzbach Aug 3, 2018

Choose a reason for hiding this comment

Uh oh!

WalterBright commented Aug 4, 2018

Uh oh!

WalterBright left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 6, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 commented Aug 3, 2020

Uh oh!

ghost commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants