From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932565AbdJaPKj (ORCPT ); Tue, 31 Oct 2017 11:10:39 -0400 Received: from mail-io0-f196.google.com ([209.85.223.196]:44167 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932510AbdJaPKe (ORCPT ); Tue, 31 Oct 2017 11:10:34 -0400 X-Google-Smtp-Source: ABhQp+SDqEfm9XXMfWsKUf+fj/GE4XYqiA5J3viEX1A6Suuzv8PqWOiALdJcTd7a+QpbAHuXQMLQFrKpm3e3AV7Ix68= MIME-Version: 1.0 In-Reply-To: <20171031084552.mvvn73pfew4rebsd@mwanda> References: <7b418db1-ec35-4c85-5d06-45edde56822c@users.sourceforge.net> <20171030121508.zfnsvpsrt25prhr4@mwanda> <20171031084552.mvvn73pfew4rebsd@mwanda> From: Ulf Hansson Date: Tue, 31 Oct 2017 16:10:32 +0100 Message-ID: Subject: Re: [PATCH] mmc: vub300: Use common code in __download_offload_pseudocode() To: Dan Carpenter Cc: SF Markus Elfring , "linux-mmc@vger.kernel.org" , Linux USB List , Tony Olech , LKML , kernel-janitors@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 31 October 2017 at 09:45, Dan Carpenter wrote: > On Mon, Oct 30, 2017 at 02:03:13PM +0100, Ulf Hansson wrote: >> On 30 October 2017 at 13:15, Dan Carpenter wrote: >> > On Mon, Oct 30, 2017 at 12:40:39PM +0100, Ulf Hansson wrote: >> >> On 27 October 2017 at 21:31, SF Markus Elfring >> >> wrote: >> >> > From: Markus Elfring >> >> > Date: Fri, 27 Oct 2017 21:21:40 +0200 >> >> > >> >> > Add a jump target so that a specific string copy operation is stored >> >> > only once at the end of this function implementation. >> >> > Replace two calls of the function "strncpy" by goto statements. >> >> > >> >> > This issue was detected by using the Coccinelle software. >> >> > >> >> > Signed-off-by: Markus Elfring >> >> >> >> Thanks, applied for next! >> >> >> > >> > What's the advantage of this patch? The new code seems more complicated >> > to me and GCC automatically reuses duplicate constant strings so there >> > is no memory savings. >> >> It looked to me that the error path got a bit cleaner. However, I >> guess it's matter of taste. >> >> If you insist, I can drop it. > > I'm on the kernel-janitors list so I am CC'd on all of Markus's patches. > It's not my code and I'm tired of being the anti-Markus guy so this > patch is fine. Markus has a tool that finds duplicate strings and he > uses gotos to avoid them. I don't think duplicate strings are a problem > or that it's a good idea to send over a hundred patches using this > method. But many people have explained that to Markus already and > that's not the bigger picture which is about error handling and labels. > > What I like are labels that are necessary and self explanatory. Things > like "goto unlock" are a good example, because we know we need to unlock > and the goto tells us what the label does. Or here is another example: > > foo = alloc_foo(); > if (!foo) > return -ENOMEM; > > bar = alloc_bar(); > if (!foo) { > err = -ENOMEM; > goto free_foo; > } > > From reading the code, you know that you have to free foo and the label > name is clear so you literally don't need to scroll down to the bottom > of the function when you're reading this code. A bad label is like > this: > > foo = alloc_foo(); > if (!foo) > goto err; > > You're reading along and you're like "what happens at the err" label? > So then you have to scroll down to the bottom and read the code, then > you have to think about how the variables line up with the variables > in the above code, then you have to scroll back and find your place > again and by that point you've forgotten what you were doing when you > started. Thanks for the details! Much appreciated! Regarding posts from Markus' I am well aware of the problems. In my case, it hasn't been much of a hurdle, I simply ignore changes that seems silly to me. Kind regards Uffe