Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

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

Closed
wants to merge 1 commit into from

6 participants

Andrej Mitrovic Alex Rønne Petersen Hara Kenji JakobOvrum Andrei Alexandrescu Vladimir Panteleev
Andrej Mitrovic
Collaborator

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

This supersedes #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.

2) 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.

Andrej Mitrovic
Collaborator

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.

Andrej Mitrovic
Collaborator

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.

Alex Rønne Petersen

Let us get this into the next release.

Hara Kenji
Collaborator

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() {} }
    
Andrej Mitrovic
Collaborator

@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.

Hara Kenji
Collaborator

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.");
Andrej Mitrovic
Collaborator

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: #1073

Andrej Mitrovic AndrejMitrovic closed this March 14, 2014
JakobOvrum

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 :(

Andrej Mitrovic
Collaborator

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

Andrei Alexandrescu
Owner

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

Vladimir Panteleev

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.

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

Showing 1 unique commit by 1 author.

Jan 27, 2013
Andrej Mitrovic Fixes Issue 9408 - Make invariant non-const by default and settable t…
…o const.
011756b
This page is out of date. Refresh to see the latest.
2  src/declaration.h
@@ -866,7 +866,7 @@ struct SharedStaticDtorDeclaration : StaticDtorDeclaration
866 866
 
867 867
 struct InvariantDeclaration : FuncDeclaration
868 868
 {
869  
-    InvariantDeclaration(Loc loc, Loc endloc);
  869
+    InvariantDeclaration(Loc loc, Loc endloc, StorageClass stc);
870 870
     Dsymbol *syntaxCopy(Dsymbol *);
871 871
     void semantic(Scope *sc);
872 872
     int isVirtual();
11  src/func.c
@@ -4129,8 +4129,8 @@ void SharedStaticDtorDeclaration::toCBuffer(OutBuffer *buf, HdrGenState *hgs)
4129 4129
 
4130 4130
 /********************************* InvariantDeclaration ****************************/
4131 4131
 
4132  
-InvariantDeclaration::InvariantDeclaration(Loc loc, Loc endloc)
4133  
-    : FuncDeclaration(loc, endloc, Id::classInvariant, STCundefined, NULL)
  4132
+InvariantDeclaration::InvariantDeclaration(Loc loc, Loc endloc, StorageClass stc)
  4133
+    : FuncDeclaration(loc, endloc, Id::classInvariant, stc, NULL)
4134 4134
 {
4135 4135
 }
4136 4136
 
@@ -4139,7 +4139,7 @@ Dsymbol *InvariantDeclaration::syntaxCopy(Dsymbol *s)
4139 4139
     InvariantDeclaration *id;
4140 4140
 
4141 4141
     assert(!s);
4142  
-    id = new InvariantDeclaration(loc, endloc);
  4142
+    id = new InvariantDeclaration(loc, endloc, storage_class);
4143 4143
     FuncDeclaration::syntaxCopy(id);
4144 4144
     return id;
4145 4145
 }
@@ -4167,9 +4167,12 @@ void InvariantDeclaration::semantic(Scope *sc)
4167 4167
     if (!type)
4168 4168
         type = new TypeFunction(NULL, Type::tvoid, FALSE, LINKd);
4169 4169
 
  4170
+    if (storage_class && !(storage_class & (STCconst | STCundefined)))
  4171
+        ::error(loc, "invariant() can either be const or non-const.");
  4172
+
4170 4173
     sc = sc->push();
4171 4174
     sc->stc &= ~STCstatic;              // not a static invariant
4172  
-    sc->stc |= STCconst;                // invariant() is always const
  4175
+    sc->stc |= storage_class;
4173 4176
     sc->flags = (sc->flags & ~SCOPEcontract) | SCOPEinvariant;
4174 4177
     sc->linkage = LINKd;
4175 4178
 
4  src/parse.c
@@ -1286,8 +1286,8 @@ InvariantDeclaration *Parser::parseInvariant()
1286 1286
         nextToken();
1287 1287
         check(TOKrparen);
1288 1288
     }
1289  
-
1290  
-    f = new InvariantDeclaration(loc, 0);
  1289
+    StorageClass stc = parsePostfix();
  1290
+    f = new InvariantDeclaration(loc, 0, stc);
1291 1291
     f->fbody = parseStatement(PScurly);
1292 1292
     return f;
1293 1293
 }
26  test/compilable/test9408.d
... ...
@@ -0,0 +1,26 @@
  1
+class C1
  2
+{
  3
+    int x;
  4
+    const invariant()
  5
+    {
  6
+        static assert(!__traits(compiles, x = 1));
  7
+    }
  8
+}
  9
+
  10
+class C2
  11
+{
  12
+    int x;
  13
+    invariant() const
  14
+    {
  15
+        static assert(!__traits(compiles, x = 1));
  16
+    }
  17
+}
  18
+
  19
+class C3
  20
+{
  21
+    int x;
  22
+    invariant()
  23
+    {
  24
+        x = 1;
  25
+    }
  26
+}
2  test/fail_compilation/fail7369.d
... ...
@@ -1,5 +1,5 @@
1 1
 struct S7369 {
2 2
     int a;
3  
-    invariant() { a += 5; }
  3
+    invariant() const { a += 5; }
4 4
 }
5 5
 
6  test/fail_compilation/fail9408.d
... ...
@@ -0,0 +1,6 @@
  1
+class C1
  2
+{
  3
+    invariant() shared
  4
+    {
  5
+    }
  6
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.