Nishu said:
here's lil' correction... and one more doubt.
Ok, I think I see what you're trying to do. You want to create an
array of shorts, and initialize them all to the same value. If it
were an array of bytes memset() would be just the thing, but there's
no standard routine that does for shorts what memset() does for bytes.
You're assuming that short is 16 bits and long is 32 bits. Be aware
that these assumptions are *not* portable. If you're willing to live
with the fact that your code will break, perhaps quietly, on some
systems, you should at least document your assumptions.
#include<stdio.h>
#include<stdlib.h>
int main(void)
{
short value; /* it is signed actually*/
Why is it signed? For what you're doing, using unsigned types would
be simpler -- ahd the value 0xFFF0 won't fit into a 16-bit unsigned
object.
"lvalue" turns out to be a poor name for this variable. I understand
that you meant it to be an abbreviation for "long value", but in C the
term "lvalue" has a specific meaning (roughly, it's an expression that
designates an object).
Again, unsigned short would probably serve your purposes better.
ptr = malloc(sizeof(short) * 8);
Thank you for not casting the result of malloc(), but an even better
way to write this is:
ptr = malloc(8 * sizeof *ptr);
And you should *always* check the result of malloc().
You're saving the value of ptr so you can free() it later. That's
good, but see below for a comment on this.
value = 0xFFF0; /* i get warning here. I don't understand why.
warning is
'=' : truncation from 'const int ' to 'short' */
The maximum value of signed short on your system is 0x7FFF; that's
what your compiler is warning you about.
lvalue = (unsigned short)value | ((unsigned short)value << 16);
If you declare value as an unsigned short, you don't need the casts.
*((long*)ptr)++ = lvalue;
*((long*)ptr)++ = lvalue;
*((long*)ptr)++ = lvalue;
*((long*)ptr)++ = lvalue;
This is the core of what you're doing, and of your problem.
You're initializing an array of 2-byte integers, but you're trying to
do it 4 bytes at a time, presumably because you think it will be
faster (more on that later). In general, this is a dangerous thing to
do, because there's no guarantee that an array of shorts will be
properly aligned as an array of longs. In this case, you happen to be
ok, because malloc() returns a result suitably aligned for any type.
If you're thinking of using this as a general technique, alignment
will eventually come back and bite you. You should also think about
the case where the length of the array is odd.
The "++" operator modifies an object. It can't be applied to
something that isn't an object -- specifically, it requires an lvalue,
an expression that designates an object. A cast yields a converted
*value*; it doesn't refer to any object. So applying "++" to the
result of a cast makes no more sense than assigning to the result of
"+":
int x;
(x + 2) = 4; /* illegal, (x + 2) is not an lvalue */
(The language *could* conceivably have defined reasonable semantics
for treating the result of a cast as an lvalue, but it didn't, and
we're stuck with that.)
A pointer increment advances the pointer by one object size,
specifically the size of the object to which it points. You want to
advance ptr by the size of a long, but it's declared to point to a
short. Casting the pointer *value* and incrementing it is, as
discussed above, illegal.
What you really want to do is take the value of ptr (of type short*),
convert it to long*, add 1 to the resulting *value*, and convert the
result back to short*:
ptr = (short*)((long*)ptr + 1);
Or you can just use the knowledge (well, assumption) that a long is
twice the size of a short, and simply do this:
ptr += 2;
Now this isn't something you can easily do as a side effect of your
assignment statement, but so what? Extreme terseness isn't always a
virtue. Do the assignment, then increment the pointer:
*((long*)ptr) = lvalue;
ptr += 2;
*((long*)ptr) = lvalue;
ptr += 2;
*((long*)ptr) = lvalue;
ptr += 2;
*((long*)ptr) = lvalue;
And we've dropped the final increment, since we're not using the value
of ptr again.
An outline of what you've done with memory allocation is:
ptr = malloc(...);
ptr1 = ptr;
/* manipulate ptr */
free(ptr1);
This is perfectly correct, but just as a matter of style you might
consider manipulating the *copy* of ptr rather than ptr itself. This
makes for greater symmetry (ptr = malloc(...); ... free(ptr)
. You
can even declare ptr as const to make (reasonably) sure you don't
accidentally clobber it.
Now let's zoom back and look at what you're trying to do.
Except in performance-critical code, it's usually best to write the
most straightforward possible code, and let the compiler optimize it
as well as it can. In your program, you write a considerable amount
of extra code to initialize your array 4 bytes at a time rather than 2
bytes at a time, and you've unrolled the initialization loop (writing
four separate assignments rather than a single one in a loop). Either
or both of these *might* make your code a little faster -- or they
might not. By making your code more complex, you may have made it
more difficult for the optimizer to analyze; conceivably a good
optimizing compiler could have done a better job than you have. And,
by trying to be fancy, you've gotten the code wrong, which has cost
you many orders of magnitude more of your own time than the CPU time
you might have saved.
Once you get your code working, try measuring the *actual* time spent;
you may find that the improvement is either nonexistent, or just not
worth the effort.
Or you just might find that it's significant, that the code is in an
inner loop in a performance-critical application, and that this was
all worth it. Or it could be worthwhile just as a learning
experience.
If you do want to do this optimization, there are easier ways to go
about it. You can treat the whole array as an array of unsigned longs
and initialize it that way, rather than converting pointers on each
iteration.
Here's a simplified version of your program. I've removed the
optimizations, but I've added some error checking.
================================
#include <stdio.h> /* This isn't actually used */
#include <stdlib.h>
int main(void)
{
#define COUNT 8
#define INIT 0xFFF0
unsigned short value;
unsigned short *const ptr = malloc(COUNT * sizeof *ptr);
int i;
if (ptr == NULL) {
fprintf(stderr, "malloc() failed\n");
exit(EXIT_FAILURE);
}
for (i = 0; i < COUNT; i ++) {
ptr
= INIT;
}
free (ptr);
return 0;
}
================================
And here's another version with optimization. I've unrolled the loop
as you did, and I've used a pointer rather than an integer index to
access the array. (It's tempting to assume that pointer accesses will
be faster than indexing, but it's not necesarily the case -- and an
optimizing compiler can often tranform one to the other.)
One note here. I've defined COUNT as a macro rather than using the
"magic number" 8. The idea is that you can change the definition of
COUNT if necessary without touching the rest of the program. For the
unoptimized version above, that works. For the optimized version
below, it doesn't; the number of assignment statements needs to be
COUNT/2 (and COUNT needs to be even), but if you change the definition
of COUNT you also need to manually update the unrolled loop.
================================
#include <stdio.h>
#include <stdlib.h>
#include <assert.h>
int main(void)
{
/*
* We require long to be twice the size of short.
* We'll check this with an assert(); if it's not the case,
* we don't even want the program to run.
*/
#define COUNT 8
#define INIT 0xFFF0
#define INIT_TWICE (((unsigned long)INIT << 16) | (unsigned long)INIT)
unsigned short value;
unsigned short *const ptr = malloc(COUNT * sizeof *ptr);
unsigned long *lptr;
assert(sizeof(short) * 2 == sizeof(long));
if (ptr == NULL) {
fprintf(stderr, "malloc() failed\n");
exit(EXIT_FAILURE);
}
lptr = (unsigned long*)ptr;
*lptr++ = INIT_TWICE;
*lptr++ = INIT_TWICE;
*lptr++ = INIT_TWICE;
*lptr = INIT_TWICE;
free (ptr);
return 0;
}
================================