Noissue - Use iterators syntax consistently

Noissue - Use iterators syntax consistently

[Wladimir Palant, 2017-04-06 08:03:22]

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:144: }
Interestingly, this change resulted in less compact code for no obvious reason.
Code generated for other functions stayed identical.

[hub, 2017-04-06 08:55:27]

LGTM

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:144: }
On 2017/04/06 08:03:22, Wladimir Palant wrote:
> Interestingly, this change resulted in less compact code for no obvious
reason.
> Code generated for other functions stayed identical.

I'd be tempted to file a bug for that. How bad is it?

[Wladimir Palant, 2017-04-06 09:36:42]

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
File compiled/filter/ElemHideBase.cpp (right):

https://codereview.adblockplus.org/29404555/diff/29404556/compiled/filter/Ele...
compiled/filter/ElemHideBase.cpp:144: }
On 2017/04/06 08:55:27, hub wrote:
> On 2017/04/06 08:03:22, Wladimir Palant wrote:
> > Interestingly, this change resulted in less compact code for no obvious
> reason.
> > Code generated for other functions stayed identical.
> 
> I'd be tempted to file a bug for that. How bad is it?

Actually, I got it backwards it seems - the code got more compact now, 37 bytes
less than it was before. If you are really interested, the original loop was:

        do {
            if ((i[b + 8 >> 0] | 0) != 0 ? (o = b + 4 | 0, p = k[o >> 2] | 0, (p
& 1073741823 | 0) != 0) : 0) {
              ...
            }
            while (1) {
                c = b + 12 | 0;
                if ((c | 0) == (l | 0)) {
                    b = l;
                    break
                }
                if ((k[b + 16 >> 2] | 0) >>> 0 > 3221225471) b = c;
                else {
                    b = c;
                    break
                }
            }
            h = k[m >> 2] | 0
        } while ((b | 0) != ((k[h >> 2] | 0) + ((k[h + 4 >> 2] | 0) * 12 | 0) |
0));
        return;

And this is the new one:

        b: while (1) {
            if ((i[b + 8 >> 0] | 0) != 0 ? (n = b + 4 | 0, o = k[n >> 2] | 0, (o
& 1073741823 | 0) != 0) : 0) {
              ...
            }
            do {
                c = b;
                b = b + 12 | 0;
                if ((b | 0) == (l | 0)) {
                    b = 25;
                    break b
                }
            } while ((k[c + 16 >> 2] | 0) >>> 0 > 3221225471);
            if ((b | 0) == (l | 0)) {
                b = 25;
                break
            }
        }
        if ((b | 0) == 25) return

All in all not a huge difference, simply the optimizer taking a slightly
different approach when inlining HashContainerIterator::operator++. I do wonder
however where that 25 constant is coming from and what the point of it is - this
is end of function, so a conditional return makes no sense whatsoever at this
point. Never mind the fact that b will *always* be set to 25 when exiting this
loop.

[Wladimir Palant, 2017-04-06 10:00:44]

On 2017/04/06 09:36:42, Wladimir Palant wrote:
> I do wonder
> however where that 25 constant is coming from and what the point of it is -
this
> is end of function, so a conditional return makes no sense whatsoever at this
> point. Never mind the fact that b will *always* be set to 25 when exiting this
> loop.

Ok, a debug build shows that this is the Emscripten way of translating jumps -
each jump gets a unique label value (in this case it is 25) and is translated
into an assignment to the label variable and break. After the loop the label
variable is checked in order to see whether a jump was executed. Here this
mechanism generated some unnecessary code, might actually be worth filing a bug
about that.

[Wladimir Palant, 2017-04-06 11:21:15]

I managed to reduce the code and filed an Emscripten issue:
https://github.com/kripken/emscripten/issues/5115