Date: Sun, 12 Jan 2003 01:46:44 -0500 From: Jake Burkholder <jake@locore.ca> To: Matthew Dillon <dillon@apollo.backplane.com> Cc: cvs-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern kern_acl.c kern_descrip.c kern_event.c kern_mac.c sys_pipe.c sys_socket.c uipc_socket.c uipc_syscalls.c uipc_usrreq.c vfs_aio.c vfs_syscalls.c vfs_vnops.c src/sys/netgraph ng_socket.c src/sys/sys file.h src/sys/ufs/ffs ... Message-ID: <20030112014644.F212@locore.ca> In-Reply-To: <200301120254.h0C2srcS043241@apollo.backplane.com>; from dillon@apollo.backplane.com on Sat, Jan 11, 2003 at 06:54:53PM -0800 References: <200301120137.h0C1bD0E098037@repoman.freebsd.org> <20030111213259.E212@locore.ca> <200301120254.h0C2srcS043241@apollo.backplane.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Apparently, On Sat, Jan 11, 2003 at 06:54:53PM -0800, Matthew Dillon said words to the effect of; > :actually needed is in cases like this: ((struct vnode *)fp->f_data)->v_mount. > :In the majority of cases that I looked at the pointer is assigned to a > :local variable of the correct type first, in which case the cast is almost > :certainly a historical remnant of f_data having type caddr_t. Removing the > :unnecessary casts in that case I agree with, but changing the type of f_data > :as well I don't agree with at all. > > Well, a good bit of the casts removed also allowed the parenthesis to > be removed, a couple of split lines could be put back together, and > generally it has made it more readable. Exactly 1 line unless I completely failed in generating an accurate diff of your commit. The lines would be shorter if you had just removed the unnecessary casts. I find the code more readable with just the unnecessary casts removed. > > :This is also a layering violation. The file layer is not supposed to > :know what the underlying object is. I understand that the generic pointer > :deals with that, but I don't see how f_data was insufficient and I think > :its wrong to specify in anyway other than void * what the underlying > :object is. > > The structs are there but the structural definitions are not (i.e. there > is no #include requirement), so I would disagree about it being a > layering violation. Also, nearly all the code that uses struct file > assumes that f_data is one type or another... in otherwords, there is > no 'layering' per-say to violate. Most of the code in kern/* needs to > know exactly what f_data is. Ok. > > :I'm not at all happy with this change and I would like some justification > :for it other than to remove casts; in the majority of cases they are > :unnecessary and should have been just removed, and the remaining cases > :would have been better served by first assinging to a local variable of the > :correct type. > : > :I'm really baffled as to why you did this. > : > :Jake > > It's a cleanup. Ugliness I noticed as I was evaluating how easy it > would be to add a memory object file type. I don't see why you believe > a justification is required other then cleanup and code readability. > void *'s are things you only use when you need to. We certainly didn't > need to here. How exactly is this commit so bad that you are against it? Its unnecessary code churn. I wouldn't be surprised if struct file was used all the place in 3rd party modules, especially MAC modules. Now they either need an ugly compat define or ugly idfefs. Some of your commits were to code that's externally maintained. We've broken compatibility with external code before for compelling reasons. This is not a compelling reason. You obviously don't think this is important, I do. If no one else cares about this I'll shut up now. Jake To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20030112014644.F212>