Hi Adis,
On Thu, Jan 23, 2020 at 10:40:12AM +0100, Adis Nezirovic wrote:
> On 1/22/20 6:17 PM, Willy Tarreau wrote:
> > Strangely, while I was certain I had build-tested the original one,
> > apparently
> > I failed as it didn't build for two errors, which I fixed in a subsequent
> > patch.
> > I'm seeing that your patches still seem to rely on this bug (data_op < 0)
> > which
> > normally cannot compile so I find this surprising.
>
> I've using clang during the testing, and apparently -Wextra doesn't enable
> it on clang (I'm using v9.0), only -pedantic helps:
>
> clang -Wall -Wextra -Wpedantic test.c
>
> test.c:6:12: warning: ordered comparison between pointer and zero ('char *'
> and 'int') is an extension [-Wpedantic]
Ah that's interesting, because from a semantic perspective there is no
reason to compare an int and a pointer, given that an int is a distance
between two pointers (divided by the element's size), so being able to
do that implies that it is possible to add two pointers, which makes no
sense:
let i := p1 - p2
for (i < p3) to be true, this would mean:
p1 - p2 < p3
p1 < p2 + p3 :-)
And going further we might even multiply pointers to create areas or
volumes maybe! Compilers will never cease to amaze me! At least this
explains the difference between what you saw and what I saw.
> Sorry about that, after being away from C for a while, it's hard to switch
> on some brain, instead of letting the compiler to catch stupid bugs like
> this one.
Hey don't be sorry, we all do bugs and the compiler cannot catch them all
(and I'd actually like that it tries less so that it doesn't report false
positives all the time that require to add dangerous code to shut it up).
My comment was really about pulling the alarm trigger in case there was
something wrong in your patchset that you didn't notice.
> > I still merged the first one of these two because I think it's OK, however,
> > could you please add a bit of commit message to the second one ? As a rule
> > of thumb, the commit message should be used to "sell" me your patch and to
> > sell it to whoever would be hesitating in backporting it or not. Thus I
> > think
> > that here the benefits and/or impacts are not obvious from this one-liner :
>
> Here is me doing my best, selling you the good stuff, and the reason to
> backport the whole feature (so we can detect the multi-filter from CLI, and
> not rely on haproxy version or similar), see the attached patch.
:-)
Pretty nice, now merged.
Thank you!
Willy
