How to get rid of const / immutable poisoning (and I didn't even want to use them...)

[ag0aep6g via Digitalmars-d-learn]

On 05/22/2017 10:13 PM, Enjoys Math wrote:
> immutable struct String(T) {

Marking a whole struct as `immutable` is usually a mistake. It applies 
to all methods, making them unusable on mutable and const instances.

> public:
>      this(immutable T[] s) {

This means you can create `String`s only with immutable arrays. Again, 
not what you want usually. You can use `inout` to make a constructor 
that accepts mutable, const, and immutable arrays when constructing 
mutable, const, and immutable instances, respectively:

----
this(inout T[] s) inout { ... }
----

If `inout` doesn't work for some reason, you will have to define 
distinct constructors:

----
this(T[] s) { ... }
this(const T[] s) const { ... }
this(immutable T[] s) immutable { ... }
----

>          this.s = s;
>      }
> 
>      alias s this;
> 
>      string toString() const { return to!string(s); }
>      string flattened() const { string t = "";  foreach (c; s)    t ~= 
> to!string(c);  return t; }
> 
>      size_t toHash() const @system pure nothrow
>      {
>          return hashOf(flattened());
>      }
> 
>      bool opEquals(ref const String t) const @system pure nothrow
>      {
>          if (t.length != this.length)
>              return false;
> 
>          for(size_t k=0; k < t.length; k++)

Off topic: foreach (k; 0 .. t.length)

>              if (t.s[k] != this.s[k]) return false;

Seeing how `k` is used, could also make it:

----
foreach (k, e; t)
     if (e != this.s[k]) return false;
----

>          return true;
>      }
> 
>      // A compressible is a substring that occurs (non-overlappingly) 
>  >=2 and has a length >= 3
>      // or one that occurs >= 3 and has a length of 2
>      String[] compressibles() const {
>          auto count = compressibleCount();
>          String[] substrings;
>          foreach (s, n; count)
>              substrings ~= s;
>          return substrings;
>      }
> 
>      size_t[String] compressibleCount() const {
>          auto count = substringCount(2, size_t(s.length / 2));

Off topic: substringCount's second parameter is an int, not a size_t. 
Converting from size_t to int like this works with -m32, but if large 
values are possible, you've got a bug. With -m64 it simply doesn't work. 
You can use `to!int` which throws on failure.

>          foreach (str, n; count)
>              if (str.length == 2 && n < 3 || str.length >= 3 && n < 2)
>                  count.remove(str);
> 
>          return count;
>      }
> 
>      String[] substrings(int minLen=0, int maxLen=-1) const {
>          auto count = substringCount();
>          String[] substrings;
>          foreach (s, n; count)
>              substrings ~= s;
>          return substrings;
>      }
> 
> 
>      size_t[String] substringCount(int minLen=0, int maxLen=-1) const {
>          if (maxLen == -1)
>              maxLen = s.length;

Same bug-prone conversion from size_t to int as above.

>          assert (maxLen <= s.length && minLen >=0 && minLen <= maxLen);
> 
>          size_t[String] count;
> 
>          if (minLen == 0)
>              count[empty] = s.length + 1;

`empty` is not a String, it's a T[]. Works when you change `empty` to 
`static empty = String([]);`.

>          for (size_t i=0; i < s.length - minLen; i++) {
> 
>              size_t max_len = min(s.length - i, maxLen);
> 
>              for (size_t l=1; l < max_len; l++) {
>                  auto substr = String(this.s[i..i+l]);

We're in a const method, so `this.s[i..i+l]` is const as well. If you 
want to return a result with mutable keys, you have to `.dup` that slice 
of s.

Alternatively, drop `const` from the method, or return a result with 
const keys. Though returning `const` has similar repercussions as 
returning immutable keys (next paragraphs).

Arguably, keys of associative arrays should be immutable. When the keys 
change and the associative isn't re-hashed, it's going to behave 
erratically.

Making the keys immutable would mean you have to `.idup` the slices. 
You'd also have `.dup` in `compressibles` and `substring` if they're 
supposed to return mutable `String`s.

>                  if (!(substr in count))
>                      count[substr] = 0;
>                  else
>                      count[substr] ++;
>              }
>          }
> 
>          return count;
>      }
> 
> public:
>      immutable(T)[] empty = [];
> 
> private
>      T[] s;
> }