Is it bad practice to work on a structure and assign the result to the same structure? What for?
I don't remember to see code examples like this hypothetical snippet:
cpu->dev.bus->uevent = (cpu->dev.bus->uevent) >> 16; //or the equivalent using a macro
in which an element in a large structure is dereferenced using pointers operated on, and the result is assigned back to the same structure field.
The kernel seems to be the place where such large structures are common, but I have not seen examples of this and have become interested for the reason why.
Is there a reason for this, possibly related to the time it takes to track pointers? It's just not a good style, and if so, which one is preferable?
source to share
It has more to do with history: the kernel is mostly written in C (not C ++), and in the original development intent (the K&R era) was considered a "high-level assembler" whose statement and expression must have a literal correspondence in C and ASM. In this environment, ++i
i+=1
and i=i+1
are completely different things that translate into completely different CPU instructions
Compiler optimizations, at a time when they were not so advanced and popular, so the idea of following a chain of pointers should often be avoided by first storing the resulting destination address in a local temporary variable (most likely a register
) and rather than the destination.
(for example int* p = &a->b->c->d; *p = a + *p;
)
or trying to use the compond command for example a->b->c >>= 16;
)
Currently, computers (multi-core processors, tiered caches, and pipelines) executing coded internal registers can be ten times faster than accessing memory, after three pointers, faster than storing an address in memory, thereby returning the "business model" priority.
The compiler optimization is then free to alter the resulting code to suit size or speed, whichever remains more important and depends on which processor you are working with.
So now it doesn't matter if you write ++i
either or i+=1
or i=i+1
: the compiler will most likely issue the same code trying to access it i
only once. and the following chain of pointers will most likely be rewritten twice as equivalent (cpu->dev.bus->uevent) >>= 16
since it >>=
corresponds to the same machine instruction in x86 derived processors.
That said ("it doesn't really matter"), it is also true that the style of the code tends to reflect the styles and fashions of the time it was first written (since later developers tend to maintain consistency).
You code is not "bad" in and of itself, it just looks "strange" in the place that is usually written.
Just to give you an idea of what piping and prediction are. consider a comparison of two vectors:
bool equal(size_t n, int* a, int *b)
{
for(size_t i=0; i<n; ++i)
if(a[i]!=b[i]) return false;
return true;
}
Here, as soon as we find something different, we sort by saying they are different.
Now consider the following:
bool equal(size_t n, int* a, int *b)
{
register size_t c=0;
for(register size_t i=0; i<n; ++i)
c+=(a[i]==b[i]);
return c==n;
}
There is no shortcut, and even if we find a difference, keep looping and counting. But by removing if
from the inside of the loop, if n
not that big (even less than 20), it could be 4 or 5 times faster!
An optimized compiler can even recognize this situation - has proven there are no side effects - you can rewrite the first code in the second!
source to share
I don't see anything wrong with this, it looks harmless like:
i = i + 42;
If you are accessing data items a lot, you might consider something like:
tSomething *cdb = cpu->dev.bus;
cdb->uevent = cdb->uevent >> 16;
// and many more accesses to cdb here
but even then I would like to leave it to the optimizer, which generally performs better than most people :-)
source to share
There is nothing going wrong doing
cpu->dev.bus->uevent = (cpu->dev.bus->uevent) >> 16;
but depending on the type uevent
, you need to be careful when you shift so that you don't accidentally flip unexpected bits into your value. For example, if it is a 64 bit value
uint64_t uevent = 0xDEADBEEF00000000;
uevent = uevent >> 16; // now uevent is 0x0000DEADBEEF0000;
if you thought you shifted a 32-bit value and then passed a new one uevent
into a function with a 64-bit value, then you won't miss 0xBEEF0000 as you might expect. Since the sizes match (the 64-bit value is passed as a 64-bit parameter), you won't get any compiler warnings here (which you would if you passed the 64-bit value as a 32-bit parameter).
It is also interesting to note that the above operation, similar to
i = ++i;
which is undefined (see http://josephmansfield.uk/articles/c++-sequenced-before-graphs.html for details), is still well defined as there are no side effects in the right expression.
source to share