Re: [PATCH] intel-iommu: fix build with CONFIG_BRANCH_TRACER=y

[Ingo Molnar]

* David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:

> On Tue, 2009-04-07 at 14:14 +0200, Ingo Molnar wrote:
> > Well, i consider it a feature that it flags weird if (x, y) 
> > constructs: and yes, these iterators you introduced, while they are 
> > legit C, definitely count as 'weird'. If regular code was doing it, 
> > not a loop abstraction, i'd call it non-obvious and borderline 
> > broken straight away.
> > 
> > We should _never ever_ put comma statements into if () 
> > constructs without a _really_ good reason - and if yes, we can 
> > flag that we know what we are doing, via extra parentheses.
> 
> I disagree. I don't think we should be declaring valid C syntax as 
> 'off limits', however rare it is.
>
> _Especially_ if it only actually fails with a fairly esoteric 
> config option set. That's just asking for build breakage.

It failed within 10 minutes of testing for me. And i did not say 
'off limits', i said 'needs a second look and a i-know-what-i-am 
doing flag'.

Anyway, we apparently disagree about what constitutes acceptable 
code and what not. I am more cautious about "weird looking" 
constructs, and for a good reason: often a 'hm, this looks a bit 
weird' sub-conscious feeling is the only sign we have of a serious 
bug in the making.

So making code not look weird and teaching people to not use weird 
looking patterns in usual code is a prime goal - at least to me. 

Does such an approach limit the C language? Yes, of course it does, 
that's the whole point. You appear to be more of the "if it's valid 
C it's fine" camp.

> > and if yes, we can flag that we know what we are doing, via 
> > extra parentheses.
> 
> That's hardly much of a barrier. The requirement to sprinkle 
> gratuitous-looking extra parentheses around the place really isn't 
> going to give us much of a _benefit_ in return for the build 
> breakage.

The thing is, the branch-tracer might be even more weird than your 
iterator (it certainly is, and we might even remove it if it's 
causing undue problems), but it has been upstream for some time 
already and it is certainly useful for certain types of 
difficult-to-analyze codepaths.

Also, it is not breaking the build currently [with any given 
combination of weird .config combos] - so there's no 'sprinking' 
anywhere except your arguably under-tested iterator ;-)

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/