Cannot understand C source and it does not compile in GCC, but in Visual C++

Question

In GCC I got the following error:

aes.c: In function ‘copy_block’:
aes.c:278: error: lvalue required as increment operand
aes.c:278: error: lvalue required as increment operand

This is the piece of code:

static void copy_block( void * d, void *s, uint_8t nn )
{
    while( nn-- )
        *((uint_8t*)d)++ = *((uint_8t*)s)++;
}

I tried to change it to a compileable version, but unfortunately for me as a Java programmer, it's not clear what is really happening here.

Maybe someone has an idea how I can change the source that it is compileable in GCC or someone has an idea what is happening here in detail. For me it seems wierd with the dereferencing of the left hand value, but somehow it seems to work perfectly in Visual C++.

This is a small legacy program, which I have to port to a Linux machine.

thank you for your help in advance.

I think you want `uint8_t`... – Christoph Jan 31 '11 at 12:26 — Christoph, Jan 31 '11 at 12:26

sje397 · Accepted Answer · 2011-02-01T02:07:36.720

6

Try this:

#include <string.h>
static void copy_block( void * d, void *s, uint_8t nn ) {
  memcpy(d, s, (size_t)nn);
}

What's being done there isn't good.

edited Feb 01 '11 at 02:07

answered Jan 31 '11 at 11:08

sje397

41,293
8
87
103

1

That's C++ ... the post is tagged "C", so I'd stick with (size_t)nn – 246tNt Jan 31 '11 at 11:13
Why not just `#define copy_block memcpy` and make it more useful? – Chris Lutz Jan 31 '11 at 11:14
1

@Chris - I assume there is some reason for writing 'copy_block' instead of 'memcpy' in the first place - indirection for logging or something. Global replace would be even better than a #define alias. – sje397 Jan 31 '11 at 11:16
@sje397 Hehe :) But you obviously need to replace the #include with #include as well :p – 246tNt Jan 31 '11 at 11:17
eek @246tNt - ta. back into my box :) – sje397 Jan 31 '11 at 11:19
Thank you for your help! I have no idea how you could see that this is memcpy :-) – Exceptiondev Jan 31 '11 at 12:57
You have the `s` and `d` arguments to `memcpy()` around the wrong way. – caf Feb 01 '11 at 00:16

score 5 · Answer 2 · answered Jan 31 '11 at 11:08

5

This should do it:

static void copy_block(void *d, void *s, uint_8t nn)
{
    uint_8t *ud = (uint_8t *)d;
    uint_8t *us = (uint_8t *)s;

    while (nn--)
    {
        *ud = *us;
        ++ud;
        ++us;
    }
}

It's also much more readable.

answered Jan 31 '11 at 11:08

detunized

15,059
3
48
64

the casts are unnecessary and non-idiomatic in C – Christoph Jan 31 '11 at 12:28
Use this code instead! Writing utterly obfuscated code as done in the original post should be made criminal! – Lundin Jan 31 '11 at 12:47

score 3 · Answer 3 · answered Jan 31 '11 at 11:09

This is some seriously hairy looking code. Looks like there's an issue with precedence rules on the *(pointer)++ subexpressions. (that Visual C++ accepts it is somewhere between miracle and bug) It seems to be just a (terrible) memcpy reimplementation, so I suggest you use the standard version:

memcpy(d, s, nn);

I suggest doing a quick characterisation test in Visual C++ to make sure the two versions of the code do in fact do the same thing (as they appear to). You might be relying on some quirky edge case in the compiler here.

score 3 · Answer 4 · edited Jan 31 '11 at 12:40

3

The problem is that casts return rvalues, but you're trying to do an autoincrement on the pointer returned by a cast. Try this:

uint8_t *da = d, *sa = s;
while(nn--) *da++ = *sa++;

Two other notes:

nn should probably be a good deal larger than uint8_t and should probably be size_t.
With that change, this function is named memcpy and is found (probably more efficiently) in your standard library (in string.h I believe).

edited Jan 31 '11 at 12:40

Christoph

164,997
36
182
240

answered Jan 31 '11 at 11:12

Chris Lutz

73,191
16
130
183

@Christoph - Not at all. Now it looks less like I wrote it on my iPhone. :P – Chris Lutz Jan 31 '11 at 20:49

score 1 · Answer 5 · answered Jan 31 '11 at 11:08

1

You probably screwed up the parenthesis position. You want to increment the pointer, not the dereferenced value.

static void copy_block( void * d, void *s, uint_8t nn )
{
    while( nn-- )
        *((uint_8t*)d++) = *((uint_8t*)s++);
}

As a bonus tip, use memcpy ... much faster !

answered Jan 31 '11 at 11:08

246tNt

2,122
1
16
20

1

Postfix `++` has higher precedence than unary `*` unless I'm sorely mistaken. – Chris Lutz Jan 31 '11 at 11:13
And ? Postfix ++ means the 'evaluated' value will be the 'before' value. So the value given to the '*' dereferencing will be the value before increment. – 246tNt Jan 31 '11 at 11:15
Or maybe you just meant the ( ) are useless ? Yes they are ... I just left them to show the 'minimum' modification. – 246tNt Jan 31 '11 at 11:16
No, the parenthesis position was correct. Incrementing void pointer is not defined (but gcc accepts it) and it only accidentally happens to use the same size (1) for both. – Jan Hudec Jan 31 '11 at 11:17
1

No, postfix `++` is higher precedence, which means it binds tighter than `*`, which means you didn't need to parenthesize anything. The code still doesn't work because you're using the increment operator on an rvalue, which is what the error stated. – Chris Lutz Jan 31 '11 at 11:20
3

That's a lot of arguing over something that supposed to be basic feature of a language. Maybe it's a good idea not to write code in such way that it is easy to misread it. – detunized Jan 31 '11 at 11:24
@detunized: `*dest++ = *src++` is from K&R and many C programmers are familiar with this idiom; however, understanding of rvalue/lvalue semantics seems to be lacking... – Christoph Jan 31 '11 at 12:49
1

K&R, like anyone programming in the 70s, wrote code like they did because they wanted to *save hard drive space occupied by the source code files*. Unless a few more bytes of source code on your Terrabyte HD is a major concern of yours, don't write obfuscated code. Use the version posted by detunized below, it should be completely equivalent performance-wise. – Lundin Jan 31 '11 at 12:54
@Chris ... mmm the code _does_ compile and works as expected ! – 246tNt Jan 31 '11 at 12:55
1

@246tNt: you're missing the `-pedantic` flag: your code makes use of non-standard compiler-extensions... – Christoph Jan 31 '11 at 12:59
For reference * and ++ have the same precedence actually, and just a "right to left" associativity. – 246tNt Jan 31 '11 at 13:02
@Christoph ? What extension. `*((uint8_t*)d++)` => evaluate d, increment d, cast the 'before' value to `(uint8_t *)`, dereference it. – 246tNt Jan 31 '11 at 13:04
1

@246tNt: postfix-increment has higher precedence than casting, ie you increment a `void *`, which is non-standard; `gcc -pedantic` tells you `warning: wrong type argument to increment`; `clang -pedantic` is more explicit: `warning: use of GNU void* extension [-Wpointer-arith]` – Christoph Jan 31 '11 at 13:12
`void` pointers are invalid in arithmetic. Thus `++` does not work on them. – R.. GitHub STOP HELPING ICE Jan 31 '11 at 17:59
@246tNt - Prefix `++` and postfix `++` have different precedences. Prefix `++` has the same precedence as all other prefix operators, including casts and dereferences. Postfix `++` is higher. Your code compiles because GCC allows pointer arithmetic on `void *` pointers, which is non-standard. – Chris Lutz Jan 31 '11 at 20:54

score 0 · Answer 6 · answered Sep 29 '22 at 08:59

0

It's a duplicate of Why isn't the result of this cast an lvalue?

Basically casts are not l-value. It works with some compilers though. Not only old GCC, quite recent Tasking also compiled "properly" and the code worked.

answered Sep 29 '22 at 08:59

vjalle

549
4
13

score -1 · Answer 7 · answered Jan 31 '11 at 11:07

-1

Should be:

static void copy_block( void * d, void *s, uint_8t nn )
{
    while( nn-- )
        (*((uint_8t*)d))++ = (*((uint_8t*)s))++;
}

answered Jan 31 '11 at 11:07

programmersbook

3,914
1
20
13

Same problem as the original code ... no hope of working. I'm surprised visual-c++ compiled it. .. – 246tNt Jan 31 '11 at 11:12

Cannot understand C source and it does not compile in GCC, but in Visual C++

7 Answers7