Date: Sat, 11 Jan 2003 18:54:53 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Jake Burkholder <jake@locore.ca> 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: <200301120254.h0C2srcS043241@apollo.backplane.com> References: <200301120137.h0C1bD0E098037@repoman.freebsd.org> <20030111213259.E212@locore.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
: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.
: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.
: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?
-Matt
Matthew Dillon
<dillon@backplane.com>
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?200301120254.h0C2srcS043241>