4

Working with GNU/GCC

With this code :

static uint8_t write_idx=0;
write_idx = write_idx==127 ? 0 : ++write_idx;

I get this warning :

warning: operation on 'write_idx' may be undefined [-Wsequence-point]

Do you know what generates this warning ?

EDIT : 

write_idx = write_idx==127 ? 0 : write_idx+1;

Same behaviour but no warning thanks @pmg

GabrielT
  • 397
  • 6
  • 17
  • Does this answer your question? [-Wsequence-point warning - what does it mean and does it violate ANSI C standard?](https://stackoverflow.com/questions/40677318/wsequence-point-warning-what-does-it-mean-and-does-it-violate-ansi-c-standard) – jordanvrtanoski Mar 04 '21 at 14:25
  • 2
    `write_idx = write_idx==127 ? 0 : ++write_idx;` ... Undefined Behaviour trying to change `write_idx` on the left side of the `=` **and** on the right side. Suggestionr: use `if (write_idx == 127) write_idx = 0; else ++write_idx;` – pmg Mar 04 '21 at 14:26
  • 1
    The TL;DR is: Don't try to cram as many operators as you can on a single line. It does not lead to more efficient code, it does not read to more readable code, you don't get to win a price for most operators used. What you get is less readable code with more bugs. The correctly written code is: `if(write_idx==127) { write_idx=0; } else { ++write_idx; }` (with proper code formatting, obviously). – Lundin Mar 04 '21 at 14:36
  • I don't know how it behaves in comparison (the resulting assembly may be worse), but your operation can be re-written without any conditionals. `++write_idx; write_idx %= 128;` – StoryTeller - Unslander Monica Mar 04 '21 at 14:54
  • Modulo is too expensive in terms of CPU resources – GabrielT Mar 04 '21 at 15:10
  • 1
    @GabrielT - Is it? I had to check, but GCC seems quite content to optimize it https://godbolt.org/z/xa9bba - Granted, I'm no assembly expert, and the cmove version *could* be better, but at a glance it doesn't appear worse. – StoryTeller - Unslander Monica Mar 04 '21 at 15:48
  • 1
    @GabrielT modulo by a power of 2 usually gets optimized to avoid expensive operations. But if you could do `write_idx = (write_idx + 1) & 127;` to avoid the modulo operator. – Ian Abbott Mar 04 '21 at 16:00
  • Good to know thanks – GabrielT Mar 04 '21 at 16:21

1 Answers1

4

This can be reduced to:

write_idx = ++write_idx;

The compiler warns because there are two places writing into write_idx without a sequence point. This means the standard does not define what should happen.

Perhaps you don't know that the ++ pre-increment operator modifies the variable and you wanted to write this instead:

write_idx = write_idx==127 ? 0 : write_idx + 1;
Acorn
  • 24,970
  • 5
  • 40
  • 69