Large Capsicum patch for review.

[Pawel Jakub Dawidek]

On Wed, Feb 27, 2013 at 11:15:14PM +0100, Christoph Mallon wrote:
> On 25.02.2013 22:50, Pawel Jakub Dawidek wrote:
> > Using three types here is not inconsistent, it is a common practise.
> 
> Using three different types for the same context /is/ inconsistent.
> 
> > Check out even read(2) and write(2) - they take size as size_t, as there
> > is no need for a negative size, but they return ssize_t, because they
> > can return -1 if an error occured. This is exactly what I do in
> > cap_ioctls_get().
> 
> "Common practise" does not automatically turn something into a good idea.

This is what we use for syscalls in FreeBSD and I want my code to be
consistent with the rest of the code. We really don't want to make every
syscall to be implemented in its own special way following creation's
day trends. I also do like how read(2) and write(2) use size_t and
ssize_t.

> > I use size_t as this is preferred type for size, but I don't need size_t
> > for iterator, because I know the value will never need more than 16
> > bits, so I use int as a more CPU-friendly type.
> 
> See my earlier mail about this.

I changed int/u_int to long/u_long, but that's all I can do. Using
ssize_t/size_t as iterators types just looks odd to me.

> > The entire history is there, nothing is lost:
> > 
> > 	http://p4db.freebsd.org/
> 
> So effectively it is lost.
> FreeBSD's history is recorded in its SVN respository.
> Throwing big patch bombs into it only makes it harder to comprehend changes.

Well, by moving experimental development to perforce allowed to
stabilize FreeBSD-HEAD greately, which was a huge step forward for the
project as more people can run HEAD these days than in the past.

Now with SVN many people moved their development to SVN, but I
personally love perforce and in my opinion it is just so much better
than anything else I've tried over the years (CVS, SVN, hg, git).
I hope you agree that I'm free to choose the best tools to do my job.
Sadly it means the full history is harder to obtain, but on the other
hand history is not poluted with all small commits that don't really
help to review the patch, but introduce unnecessary bloat.

FYI, There were ~400 perforce commits during that work. I don't think
people would be happy if I'd commit them to SVN one by one.

> > Your changes were pretty simple, so they look nice as separate commits.
> 
> It's the other way round:
> The changes are simple, because they are separate commits, which do a single thing each.
> So it is clear what to look for in each commit.

No. Your changes were simple. Period. I can export all 400 changes for
you to review, but I'm pretty sure it would not make the review any
easier, quite the opposite. Especially that after reviewing first 50
changes you could realize that it was a failed experiment and the work
was thrown away later during development.

> > Trying to squeeze as much as possible into one line and then breaking
> > the line into three doesn't cure readability either, IMHO.
> > It's probably a matter of taste, but my version is more readable for me.
> 
> The version with the if-else is simply a different way to break the line.
> It just does it with more code duplication.

The point is that each line can be read separately.

> > Ok, I applied your change, after looking at it more it definiately is
> > simpler, although I don't think ?:'s result is boolean, but that's not
> > a big deal.
> 
> The result type of ?: is whatever the combined type of the second and third operand of the ?: is.

Not really.

	uint8_t a, b;

	a = b = 0;
	printf("%zu\n", sizeof(true ? a : b));

Result:	4

> The result type of all logical and comparison operators is int, so the result is int (with a hint of boolean, because the result of these operators is always 0 or 1).

Right, but something like the following makes me sad:

	int intval;
	void *ptr;
	char ch;

	if (!intval)
		;
	if (ptr)
		;
	if (ch)
		;

I much prefer explicit comparison and would love to see clang being
extended to issue a warning when non-boolean is checked:

	if (intval == 0)
		;
	if (ptr != NULL)
		;
	if (ch != '\0')
		;

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20130227/2c201436/attachment.sig>