Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Issue 9408 - Make invariant non-const by default and settable to const #1560

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@ghost

ghost commented Jan 27, 2013

http://d.puremagic.com/issues/show_bug.cgi?id=9408

This supersedes D-Programming-Language#1073

I have two problems:

  1. storage_class doesn't seem to be set when the qualifier is on the left-hand-side and it's shared, e.g.:
class C
{
    shared invariant()
    {
    }
}

It seems to work with const but not with shared, I don't know why it's not reflected in storage_class.

  1. Do shared/immutable/etc qualifiers make sense? I've added a check in semantic to ensure the invariant is either const or non-const, but maybe such a limitation is superfluous.
@ghost

ghost commented Jan 27, 2013

Also please note that Walter's change in this commit was potentially a breaking change and came with no documentation, warnings, or notes in the change log.

@ghost

ghost commented Jan 27, 2013

Furthermore, the error message for Issue 7360 is from a recent commit now:

Error: mutable method test.TestStruct.__invariant is not callable using a inout object

Which is much more descriptive than what it used to be. The improvement in the diagnostic should have been the initial cure for Issue 7360 instead of forcing invariants to be const and breaking any user-code.

Member

alexrp commented Feb 3, 2013

Let us get this into the next release.

Member

9rnsr commented Feb 4, 2013

At least there is two problems in this pull.

  1. Function attributes are rejeceted. This is definitely a bug.

    class C1 { @safe pure nothrow invariant() {} }
    class C2 { invariant() @safe pure nothrow {} }
  2. Prefix shared does not report error.

    class C { shared invariant() {} }
@ghost

ghost commented Feb 4, 2013

@9rnsr:

About the prefix, in DMD storage_class only seems to be updated when it's on the right-hand side, so I don't know how to fix this.

If you have clues or a better idea how to implement, go ahead so I can finally understand how it works. Thanks.

Member

9rnsr commented Feb 6, 2013

You can check actual storage classes after merging them.

    sc->stc &= ~STCstatic;     // not a static invariant
    //sc->stc |= STCconst;     // invariant() is always const
    ...

    if ((sc->stc | storage_class) & (STC_TYPECTOR & ~STCconst))
        ::error(loc, "invariant() can either be const or non-const.");
@ghost

ghost commented Sep 17, 2013

Is there any conclusion for this? I'd rather close it or reimplement it properly than let it stagnate here and waste the autotester's time. @andralex

Note that a discussion took place also in: D-Programming-Language#1073

@ghost ghost closed this Mar 14, 2014

Member

JakobOvrum commented Mar 14, 2014

Due to the forced bitwise constancy change I simply stopped using invariants entirely in my projects (my invariants were only logically constant), so I never noticed that this was never fixed :(

@ghost

ghost commented Mar 14, 2014

I suggest carrying the discussion over to the NG on whether the feature is wanted (and approved).

Owner

andralex commented Mar 15, 2014

I think invariants should not be checked for constness, purity etc. @WalterBright?

Owner

CyberShadow commented Mar 17, 2014

My 2 cents:

I think we should steer users away from writing code which does one thing with -release and another without it. Forbidding anything that may have side effects in e.g. assert expressions is probably not practical, but I don't think this should be encouraged either.

As for the arguments regarding D being a system programming language: one can still cast this to non-const to bypass constness if they really need to, right?

There recently was a SO question regarding function contracts: D: Const correctness - what am I doing wrong?. I think method contracts and invariants are closely related, and should have the same constness.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment