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

Ingo Molnar
2009-04-07T12:59:07Z

* David Woodhouse <dwmw2@infradead.org> 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