Re: [Rapp-dev] Pixelwise threshold and conditional pixel operations pathes

[Hans-Peter Nilsson]

> From: Willie Betschart <address@hidden>
> Date: Thu, 19 May 2016 15:52:54 +0200

> Well, the patches with pixelwise threshold and conditional
> pixop are have been following the guidelines.

Nope.  *Only since you insist*, some examples (meaning exactly
examples, not an exhaustive list of issues) from
cond-pixop-add-v3.patch.txt:

@@ -359,12 +365,18 @@ static const rapp_bmark_table_t rapp_bmark_suite[] = {
     RAPP_BMARK_ENTRY(contour_4conn_bin, "full", contour, 0,  0),
     RAPP_BMARK_ENTRY(contour_8conn_bin, "full", contour, 0,  0),
     /* rapp_cond functions */
-    RAPP_BMARK_ENTRY(cond_set_u8,  "empty",   cond_set_u8,  2,  0),
-    RAPP_BMARK_ENTRY(cond_set_u8,  "full",    cond_set_u8,  0,  0),
-    RAPP_BMARK_ENTRY(cond_set_u8,  "checker", cond_set_u8,  3,  0),
-    RAPP_BMARK_ENTRY(cond_copy_u8, "empty",   cond_copy_u8, 2,  0),
-    RAPP_BMARK_ENTRY(cond_copy_u8, "full",    cond_copy_u8, 0,  0),
-    RAPP_BMARK_ENTRY(cond_copy_u8, "checker", cond_copy_u8, 3,  0),
+    RAPP_BMARK_ENTRY(cond_set_u8,   "empty",   cond_set_u8,  2,  0),
+    RAPP_BMARK_ENTRY(cond_set_u8,   "full",    cond_set_u8,  0,  0),
+    RAPP_BMARK_ENTRY(cond_set_u8,   "checker", cond_set_u8,  3,  0),
(then the new "addc", then the old "copy" but with whitespace
changes, then the new "add")

Nothing in this *context* of the patch contains anything but a
whitespace change.  Please leave vertical space as-is in
neighboring lines.  It's even worse in rc_benchmark.c, it looks
like you changed every single RC_BMARK_ENTRY-line there.  To
cope, I'll apply the patch (locally) then use git diff -b to see
what you actually changed and apply only that.  Extra work.

HACKING says:
- Regarding whitespace and vertically aligned code features:
if you can keep the vertical alignment without changing lines
other than the one where the actual non-whitespace code change
is, please do.  Otherwise, whatever helps reading, but no
whitespace-only changes on any line.

And:
--- a/compute/vector/Makefile.am
(11 line changed where *adding one* would suffice.)

And also:
--- a/driver/rapp_cond.c
+    if (!RAPP_INITIALIZED()) {
+        RAPP_ABORT_FOR_ASSERTED_RETURNS();
+        return RAPP_ERR_UNINITIALIZED;
+    }
+
+    /* Validate arguments */
+    if (!RAPP_VALIDATE_RESTRICT_PLUS(dst, dst_dim, map, map_dim, height,
+                                     rc_align(width),
+                                     rc_align((width + 7) / 8)))
+    {
+        RAPP_ABORT_FOR_ASSERTED_RETURNS();
+        return RAPP_ERR_OVERLAP;
+    }
+
A formatting inconsistency even internally.  I guess we have
some of them before, but please don't add new ones.

I know, this is all whining about nits, but even if I'd apply
the change as-is I'd have to look at more changes than needed,
and I'd have to make sure no typos have crept in.

Other than that, it looks good at a glance.  I'll check for
technical issues separately.

> Would you like a
> single patch with both solutions?

No, I don't see why you think that'd help.  I'm just informing
you that about the delay, and for future reference.  Please just
be more careful.

>  The older patch with rapp_validate_buffer was probably not
> that strict with whitespaces.

That patch wasn't big and touching a lot of code, so it wasn't
much of an issue there, but it was roughly at the same level.

> I have forgotten that patch,
> that was intended as a nice to have feature.
> 
> I'm fine with not integrating that.

Again, the hesitation was regarding the even-earlier
"vectorization of integral images" a loong time ago.  The
archives are at
<https://lists.nongnu.org/archive/html/rapp-dev/>.

> I expected comments or
> other ideas on that work before it should considered as
> finished.

You already got them at the time and all other issues were
sorted out.  Since we now add new functions, it'll go in; we
already will have a DSO version-bump.  That reminds me: I'll
have to check if we're better off also using a version-script.

> Best regards
> Willie

brgds, H-P

> ________________________________________
> Från: Hans-Peter Nilsson <address@hidden>
> Skickat: den 19 maj 2016 14:19
> Till: Willie Betschart
> Kopia: address@hidden; address@hidden
> Ämne: Re: [Rapp-dev] Pixelwise threshold and conditional pixel operations 
> pathes
> 
> A 36 times improvement is impressive, thanks.
> 
> I'm looking at these and earlier patches right now.  It'll take
> a little longer than expected to weed out unnecessary
> reformatting (only white-space changes) in existing code and
> unnecessary inconsistencies in formatting (both regarding rules
> and within the submitted code) in your patch.  Those just make
> it harder to spot typos and review the *intended* changes.
> Please make sure to read and follow HACKING.
> 
> Your earlier rapp_validate_buffer submission will go in with
> this, and we'll make a release in days after that.  I'm on the
> fence regarding including the vectorization of integral
> functions submitted even before that; that'll probably be left
> out at this time as it'll have to be cleaned up and ISTR the
> performance improvement wasn't a priority.
> 
> Thanks also to Johan for the functional review.
> 
> brgds, H-P
>