3

I have a switch statement with multiple cases:

ConversionState state = ConversionState.Start; // enum
for(int i = 0; i < source.Length; i ++)
{
    switch(state)
    {
        case ConversionState.Start:
            state = ConversionState.Name; // <-- warning here
            name += source[i]; // source is a string
            break;
        case ConversionState.Name:
            if(source[i] == ' ') // <-- warning here
            {
                name = name.ToLower();
                if(name[0] == '/')
                    name = name.SubString(1);
                state = ConversionState.Between;
            }
            else
                name += source[i];
            break;
        case ConversionState.Between: // no code in this case statement, yet to be implemented
            break; // <-- warning here
    }
}

I get a warning on the three marked lines: "unreachable code detected". The very first statement after a case can't be unreachable, can it? My questions are:

  1. Is something wrong with my code, or is the warning wrong?

  2. If VS2015 thinks a code snippet is unreachable, does it get removed when I compile with optimizations on? If not, should I just ignore this warning?

  3. Does this warning mean that the marked line is unreachable, or that the whole case is unreachable?

PS: I know that the current code can be rewritten without switch, and that would solve the problem, but the future code that will be added will make it - MUCH - easier to maintain using a switch.

Edit (requested by amit dayama):

private enum ConversionState
{
  Start, Between, Name, Argument, Switch
}

The enum is inside the class that has the method that contains the originally posted code.

Edit 2: The very first line of the method this code is in was throw new NotImplementedException();. Apparently, this makes Visual Studio mark the first line of every case in every switch in the method unreachable, but nothing else, and, interestingly, not the entire code following the exception.

sisisisi
  • 481
  • 7
  • 17

3 Answers3

1

I have checked your code and everything compiles and builds on my side, using VS2012. I now there are a few problems with VS2015, with a lot of false positives in Error List Window.

So question 1: from my side, your code is correct.

Question 2: Not entirely sure, but believe that id does not remove it for optimization. I can be wrong, but the reason for the warning is that you address it and not the compiler. I will add comment when I found the correct answer for you.

Question 3: the line not the entire case.

PS: maybe you can try and add the curly braces into some of your if and else statements. I know they are not required for one liners, but they might be causing the specific behavior in VS2015

Theunis
  • 238
  • 1
  • 15
  • Thanks. If code optimising doesn't remove it, I think I can bear that 3 extra warnings. – sisisisi Oct 23 '15 at 19:03
  • Yes you could most probably live with it. It depends from developer to developer. Most probably will get fixed with the next patch of VS2015, well if that is where the problem originates from. – Theunis Oct 27 '15 at 06:44
1

You set the variable that is being checked in the switch statement manually as ConversionState.Start

It is fairly simple that since there is no code to alter this before the switch statement, visual studio will mark the other cases as unreachable code.

If I am not mistaken Visual Studio (or the plugin your using?) cannot see that the variable being used for the switch statement is being changed within the cases thus, throwing the warning. And to answer the rest of questions

If VS2015 thinks a code snippet is unreachable, does it get removed when I compile with optimizations on? If not, should I just ignore this warning?

As far as I know a warning is a warning. It does not interfere with the end result of your code. It warns you that something might be wrong.

Does this warning mean that the marked line is unreachable, or that the whole case is unreachable?

The whole case is unreachable. Control flow will never get in that case so all the code inside will never be executed.

EDIT From code review side, your code is really confusing. You use a specific case to run for the 0th element in the source collection. Why not just execute that code on the 0th element and start the loop from the next element?

John Demetriou
  • 4,093
  • 6
  • 52
  • 88
  • Thanks for pointing it out, I will remove `Start` and either place it before the for loop or merge it with `Name`. I don't have 15 reputation so it can't be seen if I upvote an answer, but take it as if I uptvoted it. – sisisisi Oct 24 '15 at 13:41
  • @sisisisi you have now :-P – John Demetriou Oct 24 '15 at 13:50
0

This seems weird to me. First of all I dont know what the ConversationState's propertys are. If it is from the Lync API the properties seem wrong, the actual are Active, Parked, Inactive and Terminated.

So I would do a Switch case like this

ConversionState state = ConversationState.Active;
for(int i = 0; i < source.Length; i ++)
{
  switch(state)
  {
    case ConversationState.Active:
      state = ConversionState.Name; 
      name += source[i]; 
      break;
    case ConversationState.Inactive:
      if(source[i] == ' ') 
      {
        name = name.ToLower();
        if(name[0] == '/')
          name = name.SubString(1);
        state = ConversionState.Between;
      }
      else
        name += source[i];
      break;
}

The Problem seems you are using the case form wrong. Please correct me if I'm wrong.

Nico
  • 1,727
  • 1
  • 24
  • 42
  • It's `ConversionState`, not `ConversationState`. Also, there is nothing wrong with OP's case syntax. Edit: Nice, I'm from Trier too originally ;) – InvisiblePanda Oct 23 '15 at 10:16