[DTrace-devel] RESEND Re: [PATCH 1/2] Minor code cleanup

[Kris Van Hees]

On Mon, Aug 24, 2020 at 10:22:33AM -0700, Eugene Loh wrote:
> On 08/24/2020 06:51 AM, Kris Van Hees wrote:
> 
> > On Tue, Aug 11, 2020 at 03:28:11PM -0700, Eugene Loh wrote:
> >> On 07/23/2020 02:07 PM, eugene.loh at oracle.com wrote:
> >>> From: Eugene Loh <eugene.loh at oracle.com>
> >>>
> >>> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> >
> > ... with incorporating the comments below...
> 
> Got it.  I incorporated your comments and will push.
> 
> FWIW, however, I do not understand them.  There are minor, arguably 
> related changes that may or may not be made as part of a patch, subject 
> to some judgment call regarding what is in scope of the patch and what 
> is not.  You argue that eliminating unneeded "(void)" is within scope in 
> two cases, even though those "(void)" are a common practice throughout 
> our code base and certainly within that file.  So, it would seem to me 
> that eliminating that "(void)" practice would be its own patch, rather 
> than just sticking two instances into this patch.  If we do not like 
> that "(void)" style, we should purge it systematically.

I generally encourage to get rid of the unnecessary (void)'s in code we
touch, particularly if you are making modifications to those lines already
anyway.  I have avoided doing a full removing of those (void)'s throughout
the code because it just doesn't seem to be important enough to do am explicit
patch for.  They are artifacts from the original code, and really useless.

> In contrast, "cleaning up comments" would be hard to imagine as a 
> single, coherent patch.  Typically, egregious comments would be judged 
> on a case-by-case basis.  Cleaning up one such comment as part of a 
> patch that is touching associated code would strike me as "in scope."
> 
> Not a big deal.  I changed it as you ask for the sake of getting these 
> patches completed.  Ideally, however, it'd be nice if there were some 
> way to think about this that made sense to us all.
> 
> >>> ---
> >>>    libdtrace/dt_printf.c | 14 ++++++--------
> >>>    1 file changed, 6 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/libdtrace/dt_printf.c b/libdtrace/dt_printf.c
> >>> index 03ffa8bd..4382c4bf 100644
> >>> --- a/libdtrace/dt_printf.c
> >>> +++ b/libdtrace/dt_printf.c
> >>> @@ -1549,7 +1549,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> >>>        const dtrace_probedata_t *data, const dtrace_recdesc_t *recp,
> >>>        uint_t nrecs, const void *buf, size_t len)
> >>>    {
> >>> -	char selfbuf[40], restorebuf[40];
> >>> +	char tmpbuf[40];
> > OK
> >
> >>>    	FILE *nfp;
> >>>    	int rval, errval;
> >>>    	dt_pfargv_t *pfv = fmtdata;
> >>> @@ -1580,11 +1580,10 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> >>>    			return (rval);
> >>>    		}
> >>>    
> >>> -		(void) snprintf(restorebuf, sizeof (restorebuf),
> >>> +		(void) snprintf(tmpbuf, sizeof (tmpbuf),
> >>>    		    "/dev/fd/%d", dtp->dt_stdout_fd);
> > Drop the "(void) " since that isn't necessary.
> >
> >>>    		free(dtp->dt_freopen_filename);
> >>> -		dtp->dt_freopen_filename = strndup(restorebuf,
> >>> -						   sizeof(restorebuf));
> >>> +		dtp->dt_freopen_filename = strndup(tmpbuf, sizeof(tmpbuf));
> > OK
> >
> >>>    	} else {
> >>>    		free(dtp->dt_freopen_filename);
> >>>    		dtp->dt_freopen_filename = strdup(dtp->dt_sprintf_buf);
> >>> @@ -1593,8 +1592,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> >>>    	/*
> >>>    	 * freopen(3C) will always close the specified stream and underlying
> >>>    	 * file descriptor -- even if the specified file can't be opened.
> >>> -	 * Even for the semantic cesspool that is standard I/O, this is
> >>> -	 * surprisingly brain-dead behavior:  it means that any failure to
> >>> +	 * This means that any failure to
> > While I agree that the original authors could have been a bit more polite in
> > their phrasing, I don't think changing it as part of this patch makes sense.
> > If we want to sanitize the comments in the code, we should do so in patches
> > that are specifically written for that.
> >
> >>>    	 * open the specified file destroys the specified stream in the
> >>>    	 * process -- which is particularly relevant when the specified stream
> >>>    	 * happens (or rather, happened) to be stdout.  This could be resolved
> >>> @@ -1620,7 +1618,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> >>>    		return (errval);
> >>>    	}
> >>>    
> >>> -	(void) snprintf(selfbuf, sizeof (selfbuf), "/dev/fd/%d", fileno(nfp));
> >>> +	(void) snprintf(tmpbuf, sizeof (tmpbuf), "/dev/fd/%d", fileno(nfp));
> > Drop the "(void) " since that isn't necessary.
> >
> >>>    
> >>>    	if (dtp->dt_stdout_fd == -1) {
> >>>    		/*
> >>> @@ -1635,7 +1633,7 @@ dtrace_freopen(dtrace_hdl_t *dtp, FILE *fp, void *fmtdata,
> >>>    		}
> >>>    	}
> >>>    
> >>> -	if (freopen(selfbuf, "aF", fp) == NULL) {
> >>> +	if (freopen(tmpbuf, "aF", fp) == NULL) {
> >>>    		(void) fclose(nfp);
> >>>    		return (dt_set_errno(dtp, errno));
> >>>    	}
> >>
> >> _______________________________________________
> >> DTrace-devel mailing list
> >> DTrace-devel at oss.oracle.com
> >> https://oss.oracle.com/mailman/listinfo/dtrace-devel
> > _______________________________________________
> > DTrace-devel mailing list
> > DTrace-devel at oss.oracle.com
> > https://oss.oracle.com/mailman/listinfo/dtrace-devel
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel