add -check command line switch #8972

WalterBright · 2018-11-19T08:28:32Z

An implementation of the idea behind DIP1006, but not the specifics.

https://github.com/dlang/DIPs/blob/master/DIPs/DIP1006.md

dlang-bot · 2018-11-19T08:28:33Z

Thanks for your pull request, @WalterBright!

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#8972"

Geod24 · 2018-11-19T08:39:38Z

CC @leandro-lucarella-sociomantic @mihails-strasuns-sociomantic

thewilsonator · 2018-11-19T10:04:45Z

doc tester doesn't like something there.

leandro-lucarella-sociomantic · 2018-11-19T17:50:28Z

It definitely cover our needs, thanks! The DIP should probably be updated too, although it is mostly only an update on names (switch and values) and add/remove some options.

joseph-wakeling-sociomantic · 2018-11-19T18:35:28Z

A question about behaviour: if one excludes assert does this mean that in, out and invariant checks are also effectively disabled, even if they are still requested with -check=in -check=out -check=invariant ... ?

rainers · 2018-11-19T18:58:09Z

src/dmd/cli.d

@@ -169,14 +169,30 @@ struct Usage
        Option("c",
            "compile only, do not link"
        ),
+        Option("check=[assert|bounds|in|invariant|out|switch][=[on|off]]",
+            "Enable or disable specific checks",
+            `Overrides default, -boundscheck, and -release options to enable or disable specific checks.


It currently overrides -boundscheck only if it is specified after it on the command line. I'd rather leave bounds out here, it already has two options and another setting safeonly.

We actually have boundscheck and noboundscheck. I'd like to deprecate them and just go with the more general check. I also want to add safe as a third option to check, so that all the checks are handled the same way with one switch.

The current implementation is not in line with this documentation: To match it -boundscheck has to remember its setting and change params.useArrayBounds only if not set already by -check=bounds=on|off.

We actually have boundscheck and noboundscheck. I'd like to deprecate them and just go with the more general check. I also want to add safe as a third option to check, so that all the checks are handled the same way with one switch.

If you want to go this route, I might suggest considering adding back the all and none values present in the DIP.

On the other hand, what you said afterwards contradicts this idea:

The idea is that changing one setting with -check has no effect on any other settings. It's designed so that each setting can be controlled independently of the others. Therefore, if you set check=in, then the in contracts are run, but if assert is turned off, then they'll be vacuous.

I can see someone running the in contracts with the assert turned off, after all, they may want to do their own wacky thing. I fear that if we get too clever with "surely the user wants assert on if in is on" we'll find ourselves in the future inevitably adding another switch so that becomes settable.

I don't want to be adding more switches. We've already tried several times to "package" these settings into other switches. The desire for this switch shows it obviously is inadequate.

It currently overrides -boundscheck only if it is specified after it on the command line.

Fixed.

rainers · 2018-11-19T19:00:29Z

src/dmd/cli.d

@@ -169,14 +169,30 @@ struct Usage
        Option("c",
            "compile only, do not link"
        ),
+        Option("check=[assert|bounds|in|invariant|out|switch][=[on|off]]",


Don't want to start bikeshedding, but the duplicate = looks strange. Why not use check-[assert|...]?

Because 1) we use on|off elsewhere, not +|- 2) I want to add a safe option as well 3) other options may become useful than just on|off|safe

I didn't mean to remove the on/off, but to replace the first = with -, e.g. -check-assert=on instead of -check=assert=on.

We already use = for the mv switch.

Another syntax question: is it assumed that the user will write separate -check=assert -check=in -check=out -check=bounds calls, or do we want to support a syntax like -check=assert,bounds,in,out that allows all the options to be specified in a single flag?

Perhaps in the future. For right now, I want to maximize simplicity.

WalterBright · 2018-11-19T20:12:13Z

if one excludes assert does this mean that in, out and invariant checks are also effectively disabled, even if they are still requested with -check=in -check=out -check=invariant ... ?

The idea is that changing one setting with -check has no effect on any other settings. It's designed so that each setting can be controlled independently of the others. Therefore, if you set check=in, then the in contracts are run, but if assert is turned off, then they'll be vacuous.

wilzbach · 2018-11-19T23:08:51Z

Why not use the existing work from #7980?

UplinkCoder · 2018-11-20T07:37:06Z

src/dmd/mars.d

+             */
+
+            // Check for legal option string; return true if so
+            static bool check(const(char)* p, string name, ref CHECKENABLE ce)


This seems awfully rigid, re checking for exactly =on or =off
For a first implementation it might be alight though.

leandro-lucarella-sociomantic · 2018-11-20T10:35:40Z

Why not use the existing work from #7980?

I find -release much less clear than -check, I would rather not end up with another enum just because there was a pre-existing -release option

joseph-wakeling-sociomantic · 2018-11-20T13:53:44Z

The idea is that changing one setting with -check has no effect on any other settings. It's designed so that each setting can be controlled independently of the others. Therefore, if you set check=in, then the in contracts are run, but if assert is turned off, then they'll be vacuous.

Gotcha. So (for example) if one just set check=assert then in effect only asserts in function bodies would run ... ?

I remember suggesting a body option in the discussion to turn on/off asserts in function bodies alone, but from what I recall that is non-trivial to implement. In any case it seems reasonable to always have in-body assert checks if one is supporting asserts at all.

WalterBright · 2018-11-21T06:42:57Z

So (for example) if one just set check=assert then in effect only asserts in function bodies would run ... ?

No. All asserts would run.

joseph-wakeling-sociomantic · 2018-11-21T10:39:18Z

No. All asserts would run.

Presumably not asserts in in, out and invariant contracts, since the entire contracts would be excluded?

Other than that, where else would you see assert checks existing other than in function bodies? (OK, and unittests, but I assume that's a special case anyway)

WalterBright · 2018-11-21T10:58:00Z

Presumably not asserts in in, out and invariant contracts, since the entire contracts would be excluded?

That's right.

Other than that, where else would you see assert checks existing other than in function bodies? (OK, and unittests, but I assume that's a special case anyway)

In executable code, which would be those areas you mentioned.

rainers · 2018-11-22T20:26:00Z

src/dmd/globals.d

@@ -175,6 +175,7 @@ struct Param
    CHECKENABLE useArrayBounds = CHECKENABLE._default;  // when to generate code for array bounds checks
    CHECKENABLE useAssert      = CHECKENABLE._default;  // when to generate code for assert()'s
    CHECKENABLE useSwitchError = CHECKENABLE._default;  // check for switches without a default
+    CHECKENABLE boundscheck    = CHECKENABLE._default;  // state of -boundscheck switch


While this now works as documented, I think it's a bit confusing to have both useArrayBounds and boundscheck as fields of Param. I would expect boundscheck to be a local variable in parseCommandLine().

wilzbach · 2018-11-27T11:10:42Z

Why not use the existing work from #7980?
I find -release much less clear than -check, I would rather not end up with another enum just because there was a pre-existing -release option

I was referring to the tests in #7980 as this PR currently has no tests.

WalterBright · 2018-11-28T09:02:17Z

It's not very testable because of the use of global variables, particularly around line 339 of mars.d. I've had thoughts once this is pulled to pull out that section into a separate function, and pass global.params in as a parameter. Then it would be testable with unit tests.

andralex

I understand this has also been OK by Sociomantic.

wilzbach · 2018-12-10T09:57:02Z

Why are there still no tests? This shouldn't have been merged without tests!!
Especially because a former submission of this which was ignored for a year had plenty of tests :/

WalterBright · 2018-12-10T10:24:30Z

@wilzbach please add them!

wilzbach · 2019-01-04T16:36:38Z

Why are there still no tests? This shouldn't have been merged without tests!!
Especially because a former submission of this which was ignored for a year had plenty of tests :/

Guess what: I was correct. the feature didn't work: #9195

please add them!

I'm sorry, but my main focus isn't D anymore.

WalterBright requested review from Geod24, MartinNowak and rainers as code owners November 19, 2018 08:28

WalterBright force-pushed the check-switch branch 2 times, most recently from 6290263 to 58a2df5 Compare November 19, 2018 11:20

rainers reviewed Nov 19, 2018

View reviewed changes

UplinkCoder reviewed Nov 20, 2018

View reviewed changes

WalterBright force-pushed the check-switch branch from 58a2df5 to f4945ba Compare November 21, 2018 10:17

WalterBright requested review from ibuclaw and dnadlinger as code owners November 21, 2018 10:17

WalterBright force-pushed the check-switch branch from f4945ba to 5cf441c Compare November 21, 2018 10:19

rainers reviewed Nov 22, 2018

View reviewed changes

add -check command line switch

559252e

WalterBright force-pushed the check-switch branch from 5cf441c to 559252e Compare December 7, 2018 10:45

andralex approved these changes Dec 10, 2018

View reviewed changes

andralex merged commit 987012b into dlang:master Dec 10, 2018

WalterBright deleted the check-switch branch December 10, 2018 04:19

leandro-lucarella-sociomantic mentioned this pull request Jan 4, 2019

Implement DIP1006 - more selective control over run-time checks #7980

Closed

wilzbach mentioned this pull request Jan 4, 2019

Fix Issue 19549 - -check=in=off doesn't work #9195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add -check command line switch #8972

add -check command line switch #8972

WalterBright commented Nov 19, 2018 •

edited

Loading

dlang-bot commented Nov 19, 2018

Geod24 commented Nov 19, 2018

thewilsonator commented Nov 19, 2018

leandro-lucarella-sociomantic commented Nov 19, 2018

joseph-wakeling-sociomantic commented Nov 19, 2018

rainers Nov 19, 2018

WalterBright Nov 19, 2018

rainers Nov 19, 2018

leandro-lucarella-sociomantic Nov 20, 2018

leandro-lucarella-sociomantic Nov 20, 2018

WalterBright Nov 20, 2018

WalterBright Nov 21, 2018

rainers Nov 19, 2018

WalterBright Nov 19, 2018

rainers Nov 19, 2018

WalterBright Nov 20, 2018

joseph-wakeling-sociomantic Nov 20, 2018

WalterBright Nov 21, 2018

WalterBright commented Nov 19, 2018

wilzbach commented Nov 19, 2018

UplinkCoder Nov 20, 2018

leandro-lucarella-sociomantic commented Nov 20, 2018

joseph-wakeling-sociomantic commented Nov 20, 2018

WalterBright commented Nov 21, 2018

joseph-wakeling-sociomantic commented Nov 21, 2018

WalterBright commented Nov 21, 2018

rainers Nov 22, 2018

wilzbach commented Nov 27, 2018

WalterBright commented Nov 28, 2018

andralex left a comment

wilzbach commented Dec 10, 2018

WalterBright commented Dec 10, 2018 •

edited

Loading

wilzbach commented Jan 4, 2019

add -check command line switch #8972

add -check command line switch #8972

Conversation

WalterBright commented Nov 19, 2018 • edited Loading

dlang-bot commented Nov 19, 2018

Bugzilla references

Testing this PR locally

Geod24 commented Nov 19, 2018

thewilsonator commented Nov 19, 2018

leandro-lucarella-sociomantic commented Nov 19, 2018

joseph-wakeling-sociomantic commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WalterBright commented Nov 19, 2018

wilzbach commented Nov 19, 2018

Choose a reason for hiding this comment

leandro-lucarella-sociomantic commented Nov 20, 2018

joseph-wakeling-sociomantic commented Nov 20, 2018

WalterBright commented Nov 21, 2018

joseph-wakeling-sociomantic commented Nov 21, 2018

WalterBright commented Nov 21, 2018

Choose a reason for hiding this comment

wilzbach commented Nov 27, 2018

WalterBright commented Nov 28, 2018

andralex left a comment

Choose a reason for hiding this comment

wilzbach commented Dec 10, 2018

WalterBright commented Dec 10, 2018 • edited Loading

wilzbach commented Jan 4, 2019

WalterBright commented Nov 19, 2018 •

edited

Loading

WalterBright commented Dec 10, 2018 •

edited

Loading