Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
(aside from code formatting) of a clamping function, int_clamp().
(marked inline) To avoid duplication and to enable further re-use, this
change moves the function into qemu/cutils.h.

Signed-off-by: Phil Dennis-Jordan<phil@philjordan.eu>
---
 hw/input/hid.c        | 12 +-----------
 hw/usb/dev-wacom.c    | 11 +----------
 include/qemu/cutils.h | 11 +++++++++++
 3 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/hw/input/hid.c b/hw/input/hid.c
index 76bedc1844..89f37f1bbb 100644
--- a/hw/input/hid.c
+++ b/hw/input/hid.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "ui/console.h"
 #include "qemu/timer.h"
+#include "qemu/cutils.h"
 #include "hw/input/hid.h"
 #include "migration/vmstate.h"
 #include "trace.h"
@@ -336,17 +337,6 @@ static void hid_keyboard_process_keycode(HIDState *hs)
     }
 }
 
-static inline int int_clamp(int val, int vmin, int vmax)
-{
-    if (val < vmin) {
-        return vmin;
-    } else if (val > vmax) {
-        return vmax;
-    } else {
-        return val;
-    }
-}
-
 void hid_pointer_activate(HIDState *hs)
 {
     if (!hs->ptr.mouse_grabbed) {
diff --git a/hw/usb/dev-wacom.c b/hw/usb/dev-wacom.c
index 7177c17f03..539581010e 100644
--- a/hw/usb/dev-wacom.c
+++ b/hw/usb/dev-wacom.c
@@ -32,6 +32,7 @@
 #include "hw/usb/hid.h"
 #include "migration/vmstate.h"
 #include "qemu/module.h"
+#include "qemu/cutils.h"
 #include "desc.h"
 #include "qom/object.h"
 
@@ -215,16 +216,6 @@ static void usb_wacom_event(void *opaque,
     usb_wakeup(s->intr, 0);
 }
 
-static inline int int_clamp(int val, int vmin, int vmax)
-{
-    if (val < vmin)
-        return vmin;
-    else if (val > vmax)
-        return vmax;
-    else
-        return val;
-}
-
 static int usb_mouse_poll(USBWacomState *s, uint8_t *buf, int len)
 {
     int dx, dy, dz, b, l;
diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index da15547bfb..4394f03326 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -277,6 +277,17 @@ static inline const char *yes_no(bool b)
      return b ? "yes" : "no";
 }
 
+static inline int int_clamp(int val, int vmin, int vmax)
+{
+    if (val < vmin) {
+        return vmin;
+    } else if (val > vmax) {
+        return vmax;
+    } else {
+        return val;
+    }
+}
+
 /*
  * helper to parse debug environment variables
  */
-- 
2.36.1

On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
> (aside from code formatting) of a clamping function, int_clamp().
> (marked inline) To avoid duplication and to enable further re-use, this
> change moves the function into qemu/cutils.h.

Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)? 
MIN(MAX(a, b), c) has a few advantages:
- It works with any integer types
   (so you can replace even uint_clamp() in hw/display/vga.c)
- It makes clear that b is the minimum value and c is the maximum value
   while it is not with int_clamp()
- It is already used in other places.

On Sun, 9 Jun 2024 at 11:00, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
> > Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
> > (aside from code formatting) of a clamping function, int_clamp().
> > (marked inline) To avoid duplication and to enable further re-use, this
> > change moves the function into qemu/cutils.h.
>
> Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)?
> MIN(MAX(a, b), c) has a few advantages:
> - It works with any integer types
>    (so you can replace even uint_clamp() in hw/display/vga.c)

Well, that can of course also be achieved by wrapping MIN(MAX(v, min),
max) in a new CLAMP(v, min, max) macro.

> - It makes clear that b is the minimum value and c is the maximum value
>    while it is not with int_clamp()

I guess this aspect is rather subjective. When I see a MIN(MAX(
construct I generally feel the need to triple-check to make sure which
way around it is. (The minimum value is passed to MAX, the maximum to
MIN.)

On the other hand, there is precedent and convention for a clamp()
function with that order of parameters in other APIs and programming
languages: C++ std::clamp, Java Math.clamp(), OpenCL kernels and
OpenGL shaders can both use a built-in clamp() function, etc.

> - It is already used in other places.


The CLAMP macro would be my preferred compromise, although I don't
know how keen everyone is on introducing a new macro named after a
short, generic word.

On 2024/06/10 17:50, Phil Dennis-Jordan wrote:
> On Sun, 9 Jun 2024 at 11:00, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2024/06/09 5:20, Phil Dennis-Jordan wrote:
>>> Both hw/input/hid.c and hw/usb/dev-wacom.c define identical versions
>>> (aside from code formatting) of a clamping function, int_clamp().
>>> (marked inline) To avoid duplication and to enable further re-use, this
>>> change moves the function into qemu/cutils.h.
>>
>> Wht about replacing int_clamp(a, b, c) with MIN(MAX(a, b), c)?
>> MIN(MAX(a, b), c) has a few advantages:
>> - It works with any integer types
>>     (so you can replace even uint_clamp() in hw/display/vga.c)
> 
> Well, that can of course also be achieved by wrapping MIN(MAX(v, min),
> max) in a new CLAMP(v, min, max) macro.
> 
>> - It makes clear that b is the minimum value and c is the maximum value
>>     while it is not with int_clamp()
> 
> I guess this aspect is rather subjective. When I see a MIN(MAX(
> construct I generally feel the need to triple-check to make sure which
> way around it is. (The minimum value is passed to MAX, the maximum to
> MIN.)
> 
> On the other hand, there is precedent and convention for a clamp()
> function with that order of parameters in other APIs and programming
> languages: C++ std::clamp, Java Math.clamp(), OpenCL kernels and
> OpenGL shaders can both use a built-in clamp() function, etc.

That sounds convincing. Please add the CLAMP macro.