diff mbox series

[FFmpeg-devel,01/10] lavu/internal: add hex to int functions.

Message ID 20210727144813.452917-2-george@nsup.org
State New
Headers show
Series [FFmpeg-devel,01/10] lavu/internal: add hex to int functions.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Nicolas George July 27, 2021, 2:48 p.m. UTC
Signed-off-by: Nicolas George <george@nsup.org>
---
 libavutil/internal.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Marton Balint July 27, 2021, 8:05 p.m. UTC | #1
On Tue, 27 Jul 2021, Nicolas George wrote:

> Signed-off-by: Nicolas George <george@nsup.org>
> ---
> libavutil/internal.h | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/libavutil/internal.h b/libavutil/internal.h
> index a33e8700c3..ba221438ed 100644
> --- a/libavutil/internal.h
> +++ b/libavutil/internal.h
> @@ -291,4 +291,38 @@ void ff_check_pixfmt_descriptors(void);
>  */
> int avpriv_dict_set_timestamp(AVDictionary **dict, const char *key, int64_t timestamp);
>
> +/**
> + * Test if val is between min and max, inclusive.
> + */
> +static inline int ff_between(unsigned min, unsigned max, unsigned val)
> +{
> +    return val - min <= max - min;
> +}

This seems a bit too general. What if somebody needs int and not unsigned? 
I just think it is better to not make this ff_*.

> +
> +/**
> + * Convert a hex digit to its value, -1 if not hex.
> + */
> +static inline int ff_hexchar2int(char c)
> +{
> +    if (ff_between('0', '9', c))

av_isdigit(c)

> +        return c - '0';
> +    c &= ~('a' - 'A');

c = av_toupper(c);

> +    if (ff_between('A', 'F', c))
> +        return c - 'A' + 10;
> +    return -1;
> +}
> +
> +/**
> + * Convert a hex digit to its value, -1 if not hex.
> + */
> +static inline int ff_hexpair2int(const char *s)
> +{
> +    int a, b;
> +
> +    if ((a = ff_hexchar2int(s[0])) < 0 ||
> +        (b = ff_hexchar2int(s[1])) < 0)
> +        return -1;
> +    return (a << 4) | b;
> +}
> +
> #endif /* AVUTIL_INTERNAL_H */

Thanks,
Marton
Nicolas George July 28, 2021, 8:06 a.m. UTC | #2
Marton Balint (12021-07-27):
> > +static inline int ff_between(unsigned min, unsigned max, unsigned val)
> > +{
> > +    return val - min <= max - min;
> > +}
> This seems a bit too general. What if somebody needs int and not unsigned? I

Any type smaller than unsigned will be promoted to unsigned, which is
exactly what needs to happen. It will not work for larger types, and I
think it is ok; the C standard does the same: abs() does not work for
int64_t.

> just think it is better to not make this ff_*.

We cannot hide a static inline function. If you really insist, I can
make it a macro and #undef it later, but <PUKE EMOJI>.

> > +    if (ff_between('0', '9', c))
> 
> av_isdigit(c)
> 
> > +        return c - '0';
> > +    c &= ~('a' - 'A');
> 
> c = av_toupper(c);

That would require including avstring.h from internal.h. I do not think
these simple functions are worth it.


Regards,
Marton Balint July 28, 2021, 6:07 p.m. UTC | #3
On Wed, 28 Jul 2021, Nicolas George wrote:

> Marton Balint (12021-07-27):
>>> +static inline int ff_between(unsigned min, unsigned max, unsigned val)
>>> +{
>>> +    return val - min <= max - min;
>>> +}
>> This seems a bit too general. What if somebody needs int and not unsigned? I
>
> Any type smaller than unsigned will be promoted to unsigned, which is
> exactly what needs to happen. It will not work for larger types, and I
> think it is ok; the C standard does the same: abs() does not work for
> int64_t.
>
>> just think it is better to not make this ff_*.
>
> We cannot hide a static inline function. If you really insist, I can
> make it a macro and #undef it later, but <PUKE EMOJI>.

No, fine as is, can be changed later if needed.

>>
>> av_isdigit(c)
>>
>>> +        return c - '0';
>>> +    c &= ~('a' - 'A');
>>
>> c = av_toupper(c);
>
> That would require including avstring.h from internal.h. I do not think
> these simple functions are worth it.

Then maybe add the function to avstring.h? c &= ~('a' - 'A') is anything 
but readable, at least add a comment if you don't like avstring.h.

Thanks,
Marton
Nicolas George July 28, 2021, 9:04 p.m. UTC | #4
Marton Balint (12021-07-28):
> Then maybe add the function to avstring.h?

You mean make it public? I am not against it, but I would wait for a
third opinion.

> c &= ~('a' - 'A') is anything but
> readable, at least add a comment if you don't like avstring.h.

I can add a comment, no problem. Good idea.

Regards,
Nicolas George Aug. 14, 2021, 8:14 a.m. UTC | #5
Nicolas George (12021-07-28):
> You mean make it public? I am not against it, but I would wait for a
> third opinion.

If there are no third opinion, I will stick with the conservative stance
and keep it private.

Regards,
diff mbox series

Patch

diff --git a/libavutil/internal.h b/libavutil/internal.h
index a33e8700c3..ba221438ed 100644
--- a/libavutil/internal.h
+++ b/libavutil/internal.h
@@ -291,4 +291,38 @@  void ff_check_pixfmt_descriptors(void);
  */
 int avpriv_dict_set_timestamp(AVDictionary **dict, const char *key, int64_t timestamp);
 
+/**
+ * Test if val is between min and max, inclusive.
+ */
+static inline int ff_between(unsigned min, unsigned max, unsigned val)
+{
+    return val - min <= max - min;
+}
+
+/**
+ * Convert a hex digit to its value, -1 if not hex.
+ */
+static inline int ff_hexchar2int(char c)
+{
+    if (ff_between('0', '9', c))
+        return c - '0';
+    c &= ~('a' - 'A');
+    if (ff_between('A', 'F', c))
+        return c - 'A' + 10;
+    return -1;
+}
+
+/**
+ * Convert a hex digit to its value, -1 if not hex.
+ */
+static inline int ff_hexpair2int(const char *s)
+{
+    int a, b;
+
+    if ((a = ff_hexchar2int(s[0])) < 0 ||
+        (b = ff_hexchar2int(s[1])) < 0)
+        return -1;
+    return (a << 4) | b;
+}
+
 #endif /* AVUTIL_INTERNAL_H */