gcc on Linux is warning of an assignment that removes 'const', in a situation where the function's argument is const and the function then mutates via a local pointer. Removing 'const' from the argument silences the warning and makes it clear that the argument may be changed; or if that's not true, a more explicit cast could be done, so the compiler knows that part of it is constant and part isn't. Attached patch removes 'const'.
ChrisA
I think the hash lookup should not modify the hash list. Its quite expensive to relink it on every lookup and definitely negates any intended speedup.
Interestingly, that code seems to go back all the way to the first pike commit from ulpc.
Arne
On Fri, 30 May 2014, Chris Angelico wrote:
gcc on Linux is warning of an assignment that removes 'const', in a situation where the function's argument is const and the function then mutates via a local pointer. Removing 'const' from the argument silences the warning and makes it clear that the argument may be changed; or if that's not true, a more explicit cast could be done, so the compiler knows that part of it is constant and part isn't. Attached patch removes 'const'.
ChrisA
Benchmarks!
But yeah, for the optimization to be meaningful there should be at least be a shortpath for the case where the element is already at the head (which does not move it).
Its only being used in define lookups in the cpp. It makes cpp() about 10% slower on a file that only contains #define and #if defined() statements.
arne
On Fri, 30 May 2014, Marcus Comstedt (ACROSS) (Hail Ilpalazzo!) @ Pike (-) developers forum wrote:
Benchmarks!
But yeah, for the optimization to be meaningful there should be at least be a shortpath for the case where the element is already at the head (which does not move it).
On Sat, May 31, 2014 at 1:08 AM, Arne Goedeke el@laramies.com wrote:
I think the hash lookup should not modify the hash list. Its quite expensive to relink it on every lookup and definitely negates any intended speedup.
Interestingly, that code seems to go back all the way to the first pike commit from ulpc.
And before that, to the first import of ulpc - the only changes, as far as I can see, are the name lpc_string becoming pike_string, and then (very recently) the pointers got tagged const. Now THAT is stable code!
So the question is, what does a 'const struct hash_table *' imply? Is the entire table constant, or is it allowed to have mutable entries? And if the latter, at what point should the compiler be told that it's no longer const?
ChrisA
Your patch is correct, the pointer cannot be const, as *prev and *base are modified, which are offsets of h->htable, which is 'const struct hash_entry *'.
arne
On Sat, 31 May 2014, Chris Angelico wrote:
On Sat, May 31, 2014 at 1:08 AM, Arne Goedeke el@laramies.com wrote:
I think the hash lookup should not modify the hash list. Its quite expensive to relink it on every lookup and definitely negates any intended speedup.
Interestingly, that code seems to go back all the way to the first pike commit from ulpc.
And before that, to the first import of ulpc - the only changes, as far as I can see, are the name lpc_string becoming pike_string, and then (very recently) the pointers got tagged const. Now THAT is stable code!
So the question is, what does a 'const struct hash_table *' imply? Is the entire table constant, or is it allowed to have mutable entries? And if the latter, at what point should the compiler be told that it's no longer const?
ChrisA
Chris Angelico wrote:
So the question is, what does a 'const struct hash_table *' imply? Is
Strictly speaking it only says that the direct struct hash_table element this pointer is pointing at will not be modified in any way. If the struct contains other pointers, the things these pointers point at can be modified, but not if they point back into the current struct hash_table.
The C-standard doesn't say it (AFAIK), but it is good practice that if the struct is part of an array or linked list, none of the other array or linked list elements are being modified either.
On Sat, May 31, 2014 at 2:59 AM, Stephen R. van den Berg srb@cuci.nl wrote:
Chris Angelico wrote:
So the question is, what does a 'const struct hash_table *' imply? Is
Strictly speaking it only says that the direct struct hash_table element this pointer is pointing at will not be modified in any way. If the struct contains other pointers, the things these pointers point at can be modified, but not if they point back into the current struct hash_table.
The C-standard doesn't say it (AFAIK), but it is good practice that if the struct is part of an array or linked list, none of the other array or linked list elements are being modified either.
I know what the C standard means, I was wondering what it meant in the context of Pike's data structures :) Does it make sense to declare this parameter as const and then tell the compiler that "hey, this bit over here isn't const", or is it better to mark it all as mutable?
Marking the entire argument as mutable doesn't add any warnings anywhere else, but that just means that nothing else is taking advantage of its constness in a way the compiler can recognize.
ChrisA
Chris Angelico wrote:
On Sat, May 31, 2014 at 2:59 AM, Stephen R. van den Berg srb@cuci.nl wrote:
Strictly speaking it only says that the direct struct hash_table element this pointer is pointing at will not be modified in any way. If the struct contains other pointers, the things these pointers point at can be modified, but not if they point back into the current struct hash_table.
I know what the C standard means, I was wondering what it meant in the context of Pike's data structures :) Does it make sense to declare this parameter as const and then tell the compiler that "hey, this bit over here isn't const", or is it better to mark it all as mutable?
Given the fact that we cannot seem to guarantee that even the pointed to element is not modified, it should not be marked as const.
On Sat, May 31, 2014 at 5:46 AM, Stephen R. van den Berg srb@cuci.nl wrote:
Chris Angelico wrote:
On Sat, May 31, 2014 at 2:59 AM, Stephen R. van den Berg srb@cuci.nl wrote:
Strictly speaking it only says that the direct struct hash_table element this pointer is pointing at will not be modified in any way. If the struct contains other pointers, the things these pointers point at can be modified, but not if they point back into the current struct hash_table.
I know what the C standard means, I was wondering what it meant in the context of Pike's data structures :) Does it make sense to declare this parameter as const and then tell the compiler that "hey, this bit over here isn't const", or is it better to mark it all as mutable?
Given the fact that we cannot seem to guarantee that even the pointed to element is not modified, it should not be marked as const.
That's what I thought when I did up the patch, but my knowledge of the code is from a quarter-hour of reading it over, not a decade or so of intimate experience with the project :)
ChrisA
pike-devel@lists.lysator.liu.se
-
Arne Goedeke -
Chris Angelico -
Marcus Comstedt (ACROSS) (Hail Ilpalazzo!) ＠ Pike (-) developers forum -
Stephen R. van den Berg