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

           Summary: [TDPL] Cannot use string variables in case expressions
           Product: D
           Version: unspecified
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: jmdavisProg@gmx.com


--- Comment #0 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-06-19 03:53:32 PDT ---
This program fails to compile

import std.stdio;

void main()
{
   string foo = "foo";
   string bar = "bar";

   string mrX;

   switch(mrX)
   {
      case foo:
         writeln(foo);
         break;
      case bar:
         writeln(bar);
         break;
      default:
         writeln("who knows");
   }
}


giving this error:

prog.d(12): Error: case must be a string or an integral constant, not foo

If you change the variables to int, then it works, but it doesn't work with strings.

According to TDPL, p. 72, "Usually case expressions are compile-time constants, but D allows variables too and guarantees lexical-order evaluation up to the first match." So, according to TDPL, this code should be valid, but it currently fails.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


Stewart Gordon <smjg@iname.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|rejects-valid               |spec
                 CC|                            |smjg@iname.com
           Severity|normal                      |enhancement


--- Comment #1 from Stewart Gordon <smjg@iname.com> 2011-06-19 10:39:42 PDT ---
http://www.digitalmars.com/d/1.0/statement.html#CaseStatement
"The case expressions must all evaluate to a constant value or array. They must
be implicitly convertible to the type of the switch Expression."

http://www.digitalmars.com/d/2.0/statement.html#CaseStatement
"The case expressions must all evaluate to a constant value or array, or a
runtime initialized const or immutable variable of integral type. They must be
implicitly convertible to the type of the switch Expression."

So the code is illegal.  Clearly this is a mistake in TDPL.

Though I am made to wonder why this restriction is there.  Changing to enhancement for the meantime.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


bearophile_hugs@eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs@eml.cc


--- Comment #2 from bearophile_hugs@eml.cc 2011-06-19 11:20:58 PDT ---
(In reply to comment #1)

> Though I am made to wonder why this restriction is there.

The purposes of a switch are to give a ordered syntax to manage several value cases (final switches are able to catch some bugs too), and to compile to efficient code, sometimes a complex mix of dispatch tables and hard-coded search trees (and more, if the compiler is smart, doing automatically one of the purposes of computed gotos). I think currently DMD doesn't optimize string switches a lot, but it's not hard to think about it using a hard-coded trie, some kind of digital tree, perfect hashing, etc. Compile-time constants allow to create such optimized code.

See also bug 5862

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|spec                        |rejects-valid
           Severity|enhancement                 |normal


--- Comment #3 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-06-19 12:16:52 PDT ---
Not to be rude, but I'm changing it back to a bug. If TDPL says something, and the compiler or online documentation doesn't agree, then it's a bug until Walter decides that we're not following what TDPL says. The rule is essentially that TDPL is always right unless Walter decides otherwise. It doesn't matter what the online docs say except that if they contradict TDPL, then they're also wrong until Walter decides that what TDPL says shouldn't be correct. So if anything, at the moment, it's a bug in the spec in addition to the bug in the compiler, not an enhancement. And remember that this _does_ currently work with variables of type int. So, the spec doesn't match what the compiler is currently doing anyway.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


Stewart Gordon <smjg@iname.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |spec


--- Comment #4 from Stewart Gordon <smjg@iname.com> 2011-06-19 13:45:32 PDT ---
(In reply to comment #2)
> I think currently DMD doesn't optimize string
> switches a lot, but it's not hard to think about it using a hard-coded trie,
> some kind of digital tree, perfect hashing, etc. Compile-time constants allow
> to create such optimized code.

But that doesn't mean it would have to _always_ use a hard-coded tree.  It goes without saying that an optimisation can happen only if the criteria for it to make sense are satisfied.  Let F be the overall feature being considered, and S be the subset of this feature that can be optimised in a certain way.  Why contrive F to equal S, when you can implement a non-empty F \ S just without the optimisation?

If the case values are all constant, create this tree.  Otherwise, just compare the switched value with the cases individually.

Later on, we could improve it to use a mixture of the two approaches where some but not all cases are CTCs.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #5 from bearophile_hugs@eml.cc 2011-06-19 16:23:49 PDT ---
(In reply to comment #4)

> If the case values are all constant, create this tree.  Otherwise, just compare the switched value with the cases individually.

This is possible, of course, it just requires a bit more complex compiler.

A problem: if one of your strings are not compile-time const, because of a mistake of the programmer, there is a silent and invisible loss of performance.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #6 from Stewart Gordon <smjg@iname.com> 2011-06-20 15:11:09 PDT ---
(In reply to comment #5)
> (In reply to comment #4)
> 
>> If the case values are all constant, create this tree. Otherwise, just compare the switched value with the cases individually.
> 
> This is possible, of course, it just requires a bit more complex compiler.

But the extra complexity is nothing compared to implementing the tree optimisation in the first place.

Moreover, ISTM for switches with only a few values, comparing the cases individually might be actually more efficient.  So this extra complexity might actually be needed in order not to pessimise these simpler cases.

> A problem: if one of your strings are not compile-time const, because of a mistake of the programmer, there is a silent and invisible loss of performance.

Which is to be expected.  After all, compiler optimisation is a privilege, not a right.

Though adopting the aforementioned mixture of the two approaches would mean that any loss of performance would be small.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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


Don <clugdbug@yahoo.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |clugdbug@yahoo.com.au


--- Comment #7 from Don <clugdbug@yahoo.com.au> 2011-07-08 01:03:36 PDT ---
This must be a mistake in TDPL, or just poor wording. It's true that DMD does relax the rule on compile-time strings, to include global variables which are initialized in static this(). But it doesn't include _all_ variables.

It's pretty clear that the unqualified description in TDPL ("D allows variables too") cannot be correct. What if it's a shared variable, for example?

Note that defining it as requiring a compile-time constant allows CTFE to be
used.
If variables are permitted, then the rules become more complicated, not
simpler.

Allowing variables would be an appallingly bad feature. It would mean that to
understand control flow in a function which contains a switch, you need to
check every 'case' statement to see if it's a variable, and then you need to
check if that variable can change from inside the function.
That's a huge change from the existing language, where you know that the
control expression is the only thing that affects control flow.

This is far worse for code maintenance and readability than 'goto'.

Effectively, this would remove the 'switch' statement from the language. Switch would become nothing more than syntax salt for a sequence of 'if' statements.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #8 from Jonathan M Davis <jmdavisProg@gmx.com> 2011-07-08 01:13:14 PDT ---
Well, it works with variables which are of type int. TDPL claims that it works with variables, and switch statements work with integral types and strings. So, per TDPL, switch statements should work with string variables.

Now, personally, I'm very surprised that _any_ type of variable would be permitted as the value of a case statement under any circumstances, and it wouldn't hurt my feelings one whit if it were removed from the language completely. I see no value in such a feature. However, TDPL is pretty clear about allowing variables, so it certainly didn't get in there by accident (and using integer variables _does_ currently work).

So, it's fine with me if this is declared as an errata for TDPL, but I don't think that it's a case of TDPL being unclear.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

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



--- Comment #9 from Don <clugdbug@yahoo.com.au> 2011-07-08 06:51:56 PDT ---
(In reply to comment #8)
> Well, it works with variables which are of type int.

Wow. I just checked, and you're right. That's disturbing.

Here's the code in statement.c:

        /* This is where variables are allowed as case expressions.
         */
        if (exp->op == TOKvar)
        {   VarExp *ve = (VarExp *)exp;
            VarDeclaration *v = ve->var->isVarDeclaration();
            Type *t = exp->type->toBasetype();
            if (v && (t->isintegral() || t->ty == Tclass))
            {   /* Flag that we need to do special code generation
                 * for this, i.e. generate a sequence of if-then-else
                 */
                sw->hasVars = 1;
                if (sw->isFinal)
                    error("case variables not allowed in final switch
statements");
                goto L1;
            }
        }

Note that it only allows integers AND CLASSES!
I think this is a *major* misfeature. But you're right, and I was wrong -- this
behaviour is clearly intentional for the integer case, and it makes absolutely
no sense to allow it for integers but not for strings.

So TDPL, the spec, and DMD are all different from each other.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------