1 |
On Thu, 2013-11-28 at 20:36 +0100, Sebastian Luther wrote: |
2 |
> Am 28.11.2013 15:47, schrieb Brian Dolbec: |
3 |
> > I like this one much better, thank you. |
4 |
> > |
5 |
> > Could you rebase the 2 patches together, this one shows removing |
6 |
> > lines from the first. Also see inline comments below :) |
7 |
> |
8 |
> The removed lines are another large if-block that already existed. |
9 |
> This has been moved into _ignore_dependency() too. |
10 |
> |
11 |
|
12 |
Hey, all the more reason to move it to a function, that would have been |
13 |
4 copies of the same check otherwise. Now it can be edited in one place |
14 |
to change behavior, and do it consistently the same. |
15 |
|
16 |
> > |
17 |
> > On Thu, 2013-11-28 at 11:34 +0100, SebastianLuther@×××.de wrote: |
18 |
> >> From: Sebastian Luther <SebastianLuther@×××.de> |
19 |
... |
20 |
|
21 |
> > Since slot_operator_rebuild is already set either true or false, |
22 |
> > would it be better if "not slot_operator_rebuild" is first in the |
23 |
> > return statement. If it resolves False, it will be an early out |
24 |
> > for the remaining checks. Personally I don't know which is the |
25 |
> > more common case. But if it resolve False more often, it will be a |
26 |
> > slight speed improvement. That goes for the rest of them too, they |
27 |
> > should be ordered in the most often to resolve False order. Same |
28 |
> > for the if as well. |
29 |
> |
30 |
> This should be more often False than True. recurse_satisfied in the |
31 |
> second block should be more often True than False. |
32 |
> |
33 |
> In fact there's some duplication between the two blocks. Feel free to |
34 |
> clean this up after it has been pushed. |
35 |
> |
36 |
|
37 |
OK, thanks, I'll look to see if it can be optimized a little more. |