On Mon, May 21, 2018 at 12:33 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

On Mon, May 21, 2018 at 12:13:06AM +0300, Or Gerlitz wrote:

quoted

On Sun, May 20, 2018 at 1:20 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

On Mon, May 14, 2018 at 05:27:14PM +0300, Vlad Buslov wrote:

quoted

Substitute calls to action insert function with calls to action insert
unique function that warns if insertion overwrites index in idr.

I know this patch may be gone on V2, but a general comment, please use
the function names themselves instead of a textualized version. I.e.,
s/action insert unique/tcf_idr_insert_unique/

disagree. While doing reviews I found out that if I ask the developer
to use higher
level / descriptive language and specifically to avoid putting
variable / function names in
patch titles and change logs, the quality gets ++ big time, vs if the
developer is allowed to say

net/mlx5: Changed add_vovo_bobo()

Added variable do_it_right to add_vovo_bobo(), now we are terribly good.

In your example I agree that it is not helping and it is even allowing
such empty changelog, just as in the section I highlighted, the
descriptive language is also not helping IMHO.

I had to read it 3 times to make sure I wasn't missing a modifier word
when comparing the two functions and well, it's just saying
"Substitute calls to foo function to bar function". I don't see how
the textualized version helps in this case while, at least in this
one, I would have visually recognized the function names way faster.

Sounds like 2 bad examples for either approach.

Properly written descriptive language is maybe harder to come up with
(specifically for those of us who are not native english speakers, like me)
but is more expressive and helpful for reviews and maintenance. Lets try
to add Vlad to come up with the right higher language and not ask him to
quote function and variable named.