On 12/17/2013 12:20 PM, Francesco Chemolli wrote:
>>> >> + bool operator[](unsigned char c) const {return
>>> >> chars_[static_cast<uint8_t>(c)] == 1;}
>> >
>> > I would remove the " == 1" part to make the code simpler, avoid
>> > repeating magic constants, make it consistent with isNonZero() if that
>> > stays, and possibly make the code faster (not sure whether non-zero test
>> > is a tad faster than an equality test).
> I expect the compiler to optimise that away; it's meant to be an
> explicit cast to bool.
FWIW, my GCC is not that smart. It produces different code for "== 1"
and the implicit conversion to bool (the test program is quoted at the
end of the message):
No optimization:
> $ g++ -DEQUAL1 -O0 -S t8.cc && md5sum t8.s: e8bbc5b04d72f993fc594ae8f83773f2
> $ g++ -DBOOL -O0 -S t8.cc && md5sum t8.s: d5183e878a443c90d22ed78b19de8934
> $ g++ -DNEQUAL0 -O0 -S t8.cc && md5sum t8.s: d5183e878a443c90d22ed78b19de8934
With optimization:
> $ g++ -DEQUAL1 -O3 -S t8.cc && md5sum t8.s: 19a5c59c0c483491cc5474b25e9ccd42
> $ g++ -DBOOL -O3 -S t8.cc && md5sum t8.s: 66be706abb2b30572b9764eff6da4ed0
> $ g++ -DNEQUAL0 -O3 -S t8.cc && md5sum t8.s: 66be706abb2b30572b9764eff6da4ed0
Here is the optimized assembly difference:
> --- t8-equal1.s 2013-12-17 12:54:01.000000000 -0700
> +++ t8-bool.s 2013-12-17 12:46:56.000000000 -0700
> @@ -7,8 +7,8 @@
> .LFB443:
> .cfi_startproc
> xorl %eax, %eax
> - cmpb $1, 4
> - sete %al
> + cmpb $0, 4
> + setne %al
> ret
> .cfi_endproc
> .LFE443:
AFAICT cmpb above subtracts zero or one from the extracted chars_ value.
sete (setne) sets argument to one (or zero) depending on the result of
the comparison. I do not know whether setting/subtracting zero is faster
than doing so with one.
None of this is critically important for the Squid code in question, of
course. I was just curious.
> To avoid magic constants and make it similar
> to C, I could rework it as != 0.
OK. I am not sure we want our code to look similar to C, but it is your
call. In my tests depicted above, "!= 0" produces exactly the same code
as an implicit conversion to bool, as one would expect.
Cheers,
Alex.
> $ cat t8.cc
> #include <stdint.h>
> #include <vector>
>
> struct Foo {
>
> #ifdef EQUAL1
> bool operator[](unsigned char c) const {return
> chars_[static_cast<uint8_t>(c)] == 1;}
> #else
> #ifdef NEQUAL0
> bool operator[](unsigned char c) const {return
> chars_[static_cast<uint8_t>(c)] != 0;}
> #else
> bool operator[](unsigned char c) const {return
> chars_[static_cast<uint8_t>(c)];}
> #endif
> #endif
>
> std::vector<uint8_t> chars_;
> };
>
> int main(int argc, char **argv) {
> Foo foo;
> return foo[4] ? 1 : 0;
> }
