AVR Freaks

Don't change it.

A new improved version would not be as obviously correct.

Sometimes, you just have to accept the extra action because the "cost" is lower. This is an excellent example of that. Testing for whether or not something is already CLEARED and only CLEAR if it is SET is a common case. Testing will cost more than doing it every time. It is a very common coding "issue" and I write "issue" because (mostly) it is only a psychological issue, not a physical one. The real criterion is whether or not the test to execute or not is more or less costly than doing it every time. 

Here is an example of when it might be more costly to do it every time. Suppose that you have an I2C peripheral with some control variable in a register. Changing that variable is relatively costly because of the I2C transaction involved. So, one way to deal with this is to keep a local copy of that control variable, and test whether or not it needs to be changed. This one can sneak up on you because there is often a function such as I2CWrite(RegisterAddr,RegisterValue) that hides the cost from you. Then, it is easy for you, the programmer, to forget what the cost is and blindly put in function calls. 

But, in the OP's case, it is the other way around. The cost saving of not testing outweighs the cost of testing. 

A big question is "What do you mean by cost?" One cost could certainly be code size. For the OP's case, this cost is pretty clear. But, the case of the I2C device is less clear on this, because you "only" add a function call. Another aspect of cost is execution time. This could be a very important consideration in the I2C example. Another factor, often overlooked, is invariance of execution time through a particular branch of the program. If you have to test, then some passes will take more time than others; this cost usually is a "don't care", but sometimes it is important. Stack or SRAM usage might be another cost factor. There is no one unique expression for cost; what might be costly in one place could be little or no cost (or cost not mattering) in another. 

In the end, you have to use your brain, and evaluate the situation carefully. The rub: to evaluate carefully, you must have knowledge of the facts. That may mean lots of reading or experimenting.

Jim

Not sure what device this is, but using (pseudo-code, as I don't know the PORTE_Virt.OUT format):

if (--green_time==0) {              //on timer trigger
    if (!!(GRN_LED_PORT & GRN_LED_PIN_MASK)) {   // If green LED on
       PORTE_Virt.OUT &= ~GRN_LED_PE;       //turn LED off
       green_time=green_period-green_width; //off time
   } else {
       PORTE_Virt.OUT |= GRN_LED_PE;        //turn LED om
       green_time=green_width;              //on time
    }
}

The "if" test of LED on should compile to a sbis without adding a variable (although the compiler may do that with the previous code).