| Message ID | alpine.DEB.2.02.1402132327480.1967@localhost6.localdomain6 |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Julia Lawall <julia.lawall@lip6.fr> Date: Thu, 13 Feb 2014 23:28:27 +0100 (CET) > On Thu, 13 Feb 2014, Dave Jones wrote: > >> On Thu, Feb 13, 2014 at 10:55:23PM +0100, Julia Lawall wrote: >> >> > The patch below converts label: return; } to label: ; }. I have only >> > scanned through the patches, not patched the code and looked at the >> > results, so I am not sure that it is completely correct. On the other >> > hand, I'm also not sure that label: ; } is better than label: return; }, >> > either. If people think it is, then I can cheack the results in more >> > detail. >> >> Why not delete the label, and just replace the goto with a return if >> the label is at the end of the function ? > > Here is an example. Perhaps the uniformity of the if ... goto pattern is > valuable, though? I think it is valuable, it's so much easier to audit the return paths via a process of elimination with that kind of layout. A return in the middle of that looks out of place at best. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 Feb 2014, David Miller wrote: > From: Julia Lawall <julia.lawall@lip6.fr> > Date: Thu, 13 Feb 2014 23:28:27 +0100 (CET) > > > On Thu, 13 Feb 2014, Dave Jones wrote: > > > >> On Thu, Feb 13, 2014 at 10:55:23PM +0100, Julia Lawall wrote: > >> > >> > The patch below converts label: return; } to label: ; }. I have only > >> > scanned through the patches, not patched the code and looked at the > >> > results, so I am not sure that it is completely correct. On the other > >> > hand, I'm also not sure that label: ; } is better than label: return; }, > >> > either. If people think it is, then I can cheack the results in more > >> > detail. > >> > >> Why not delete the label, and just replace the goto with a return if > >> the label is at the end of the function ? > > > > Here is an example. Perhaps the uniformity of the if ... goto pattern is > > valuable, though? > > I think it is valuable, it's so much easier to audit the return paths > via a process of elimination with that kind of layout. A return in > the middle of that looks out of place at best. Actually, I had a student who made a tool that went the other way around, and introduced goto labels for sharable error handling code. We didn't get around to using it to send patches, though. In that tool, we didn't create labels just for returns, with the thought that in that case there was no point to introduce a goto if there was nothing to share. julia -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Julia Lawall <julia.lawall@lip6.fr> Date: Fri, 14 Feb 2014 10:58:00 +0100 (CET) > On Thu, 13 Feb 2014, David Miller wrote: > >> I think it is valuable, it's so much easier to audit the return paths >> via a process of elimination with that kind of layout. A return in >> the middle of that looks out of place at best. > > Actually, I had a student who made a tool that went the other way around, > and introduced goto labels for sharable error handling code. We didn't > get around to using it to send patches, though. In that tool, we didn't > create labels just for returns, with the thought that in that case there > was no point to introduce a goto if there was nothing to share. That's one perspective. But think of it this way, if there is a seqeuence of labels already and you're scanning for a large body of code for control transfers during an audit, what are your eyes more likely to miss? A sequence goto statements targetting well named and distinct labels or that "return" hidding there somewhere in the middle? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 14, 2014 at 01:41:37PM -0500, David Miller wrote: > From: Julia Lawall <julia.lawall@lip6.fr> > Date: Fri, 14 Feb 2014 10:58:00 +0100 (CET) > > > On Thu, 13 Feb 2014, David Miller wrote: > > > >> I think it is valuable, it's so much easier to audit the return paths > >> via a process of elimination with that kind of layout. A return in > >> the middle of that looks out of place at best. > > > > Actually, I had a student who made a tool that went the other way around, > > and introduced goto labels for sharable error handling code. We didn't > > get around to using it to send patches, though. In that tool, we didn't > > create labels just for returns, with the thought that in that case there > > was no point to introduce a goto if there was nothing to share. > > That's one perspective. > > But think of it this way, if there is a seqeuence of labels already and > you're scanning for a large body of code for control transfers during > an audit, what are your eyes more likely to miss? > > A sequence goto statements targetting well named and distinct labels > or that "return" hidding there somewhere in the middle? No argument about 'in the middle', but the suggestion upthread was for the very first case before there are any allocations etc that need unwinding. Dave -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -u -p a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c --- a/drivers/mfd/ab3100-core.c +++ b/drivers/mfd/ab3100-core.c @@ -586,7 +586,7 @@ static void ab3100_setup_debugfs(struct ab3100_dir = debugfs_create_dir("ab3100", NULL); if (!ab3100_dir) - goto exit_no_debugfs; + return; ab3100_reg_file = debugfs_create_file("registers", S_IRUGO, ab3100_dir, ab3100, @@ -623,7 +623,6 @@ static void ab3100_setup_debugfs(struct debugfs_remove(ab3100_reg_file); exit_destroy_dir: debugfs_remove(ab3100_dir); - exit_no_debugfs: return; } static inline void ab3100_remove_debugfs(void)