Code review comment for lp:~nick-dedekind/unity/lp863412.dash-result-dnd-icon

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> Looks good, just couple things:
>
> 60 + virtual ~RowAdaptorBase();
>
> Could we make this pure? Or just define it in the header, with a {}?
>

Pure virtual would require the derived classes to implement a destructor.
I don't see any benifit from inlining the destructor in a non-abstract class.

> 437 + , drag_index_(~0)
> 585 + drag_index_ = ~0;
>
> Why not just -1? As the bit complement of 0 is a bit less readable. (Though I
> like it :)
> Unless you change all the -1 usage to ~0, it should be consistent.

-1 is not an unsigned value.
I believe a comparison of "-1 == (unsigned)value" will cause an overflow error on 64bit systems.
There are a few instances of ~0 for unsigned values around the code
And they should all be changed, but I'm not going to go around and find them. :)

« Back to merge proposal