> > This function has become a bit too long now, can you please refactor it?
> > One obvious thing you could do is to extract the processing of an
> > single NB into a separate function and the parent function can take
> > care of splitting the NBL (if required) & creating the right forwarding ctx.
>
> I took it as a "function too big" issue only.
> Also, a code like this (as in OvsStartNBLIngress):
> [CODE]
> if (cond) {
> line1;
> line2;
> line;
> }
> [/CODE]
> refactored to:
> [CODE]
> line1;
> line2;
> line;
> [/CODE]
> In an ordinary git diff in CLI, this becomes difficult to track, because it's
> seen
> as removal of all lines above and add of new lines (all below). But with a
> good
> GUI git diff tool, it is seen very clear as "removed indent and associated
> if()"
>
> My intent here was to split refactor from actual bug fix, in order to improve
> reviewing - i.e. the bug fix should modify less of code, while refactor should
> keep the same functionality but present it differently. I had thought that
> putting both in one patch would make the code more difficult to review.
> Also, splitting the commits could help by having divided work: "ok, patch1 is
> ok, I'll review patch2 later".
I don't have any problems with dividing work. In fact, it would be a good
practice since it keeps the reviews smaller, easier and as a result less error
prone.
The point that I was trying to make for this specific review was -
_do_
refactor_fun0
refactor_fun1
fix_bug
_instead_of_
fix_bug
refactor_fun0
refactor_fun1
In the first scenario, the refactors are a precursor to the bug fix change,
which is the immediate beneficiary of the refactoring. The resulting bug fix is
easier to review (IMO) & is there to stay. Doing it the reverse way (second
scenario) means that the reviewer needs to review an awkwardly long function
call. And to add to the woes, the function itself will immediately be broken up
by the refactoring that follows & hence the logic will need to be revisited any
ways. All this can be avoided easily.
By the way, thanks for posting a new revision of the patch, I will take a look
at it sometime tomorrow.
Thanks,
Saurabh
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
