>> Perhaps I should refactor OvsStartNBLIngress in a separate commit.
>> I would not like to combine multiple issues in a single commit. Plus, it
>> would
>> look clearer in the history.
> I strongly prefer/advice not to do such refactoring in a separate review. It
> wasn't fun reviewing this function at all. I spent way more time reviewing it
> & at the end I was still not confident about my review. That is why I asked
> for a refactor. Doing the refactoring now has a lot more context than doing
> it at a later point. Not > to mention this unnecssarily adds more burden to
> the review effort. We spend good amount of time reviewing a long function
> call only to review it again when the same logic gets refactor. I see that
> you have already sent out a separate review for refactoring, so I will take a
> look. But I really really don't prefer this.
I'm sorry for this, Saurabh. When you said:
> 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".
Another way could have been to do the refactor first, then the bug fix. Or, if
you prefer both in the same commit, I'll make it like that.
Please tell me how you find it easier.
Thanks,
Samuel
________________________________________
From: Saurabh Shah [ssaur...@vmware.com]
Sent: Friday, August 29, 2014 9:51 PM
To: Samuel Ghinet; Alin Serdean; dev@openvswitch.org
Subject: RE: [ovs-dev] [PATCH] Create a NBL for each NB when required
Hi Samuel,
> <PATCH>
> >All NET_BUFFERs of a NET_BUFFER_LIST must go through the pipeline:
> >extract, find flow, execute. Previously, only the first NET_BUFFER of a
> >NET_BUFFER_LIST was going through this pipeline, which was erroneous.
> </PATCH>
> comments here.
>
> It might make the reading clearer, and strengthen my confidence that I did
> not miss something :)
>
Sorry about that. My client displays it nicely so I didn't realize it was
getting difficult to read. I see your point though. I find [QUOTE] [/QUOTE]
mechanism a little difficult to follow with nested comments. I will continue
using my current approach, which Ben also recommended, of adding an additional
'>' character to previous comments. In addition, I will also trim down the
content that is not relevant. Hopefully this will make it easier to see my
comments. Let me know if this still doesn't help.
> Nit: the parameter should be on a new line.
> [/QUOTE]
> I think we would also need a coding style update here: any function
> declaration or definition should have each parameter on a separate line.
> I am going to fix the modif for this function you pointed.
Hm, I was under the impression that this was already a part of the style.
Thanks for fixing it anyways.
> [QUOTE]
> 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.
> [/QUOTE]
> Perhaps I should refactor OvsStartNBLIngress in a separate commit.
> I would not like to combine multiple issues in a single commit. Plus, it would
> look clearer in the history.
I strongly prefer/advice not to do such refactoring in a separate review. It
wasn't fun reviewing this function at all. I spent way more time reviewing it &
at the end I was still not confident about my review. That is why I asked for a
refactor. Doing the refactoring now has a lot more context than doing it at a
later point. Not to mention this unnecssarily adds more burden to the review
effort. We spend good amount of time reviewing a long function call only to
review it again when the same logic gets refactor. I see that you have already
sent out a separate review for refactoring, so I will take a look. But I really
really don't prefer this.
> Yes, I think it would be a good idea to have only one style of multi-line
> comments.
> Even though, I personally prefer this style:
> /*
> * comment line
> * comment line
> */
>
I take back my earlier comment. While I still prefer the more condense form of
commenting, I think most of our code does follow the style that your prefer.
Presumably because others prefer that style too. So let's keep that going. We
should clarify this in the style guide.
Thanks,
Saurabh
PS: Hopefully this reply is easier to follow.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
