Nouveau - Sep 2022 - [PATCH v2 14/41] drm/modes: Move named modes parsing to a separate function

On Tue, 30 Aug 2022, Maxime Ripard <maxime at cerno.tech>
wrote:
> Hi,
>
> On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
>> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert at linux-m68k.org>
wrote:
>> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime at
cerno.tech> wrote:
>> >> +#define STR_STRICT_EQ(str, len, cmp) \
>> >> +       ((strlen(cmp) == len) && !strncmp(str, cmp,
len))
>> >
>> > This is not part of the move, but newly added.
>> 
>> The same construct is also duplicated elsewhere in the series, and I
>> kept being confused by it.
>
> I'm not sure what is confusing, but I can add a comment if needed.
STR_STRICT_EQ() is what's confusing. I have to look at the
implementation to understand what it means. What does "strict" string
equality mean?
>
>> The above is precisely the same as:
>> 
>> 	str_has_prefix(str, cmp) == len
>
> Here, it's used to make sure we don't have a named mode starting
with
> either e, d, or D.
>
> If I understood str_has_prefix() right,
str_has_prefix("DUMB-MODE", "D")
> == strlen("DUMB-MODE") would return true, while it's actually
what we
> want to avoid.
That's not true, str_has_prefix("DUMB-MODE", "D") ==
strlen("D") is.
> It's also used indeed in drm_get_tv_mode_from_name(), where we try to
> match a list of names with one passed as argument.
>
> With drm_get_tv_mode_from_name("NSTC", strlen("NTSC")),
we would end up
> calling str_has_prefix("NTSC-J", "NTSC") ==
strlen("NTSC-J") which would
> work. However, we end up calling prefix not a prefix, but an entire
> string we want to match against, which is very confusing to me too.
If I get this right, you have a string and you want to check if that has
a certain prefix. Additionally, you want to check the prefix is a
certain length.

Sure, that the prefix is a certain length is more of a property of the
string, which is NUL terminated later than at length, but that's doesn't
really matter.

That condition is simply str_has_prefix(string, prefix) == length.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Hi,

On Tue, Aug 30, 2022 at 04:36:23PM +0300, Jani Nikula
wrote:
> On Tue, 30 Aug 2022, Maxime Ripard <maxime at cerno.tech> wrote:
> > Hi,
> >
> > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
> >> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert at
linux-m68k.org> wrote:
> >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime at
cerno.tech> wrote:
> >> >> +#define STR_STRICT_EQ(str, len, cmp) \
> >> >> +       ((strlen(cmp) == len) && !strncmp(str,
cmp, len))
> >> >
> >> > This is not part of the move, but newly added.
> >> 
> >> The same construct is also duplicated elsewhere in the series, and
I
> >> kept being confused by it.
> >
> > I'm not sure what is confusing, but I can add a comment if needed.
> 
> STR_STRICT_EQ() is what's confusing. I have to look at the
> implementation to understand what it means. What does "strict"
string
> equality mean?
Same length, same sequence of characters
> >
> >> The above is precisely the same as:
> >> 
> >> 	str_has_prefix(str, cmp) == len
> >
> > Here, it's used to make sure we don't have a named mode
starting with
> > either e, d, or D.
> >
> > If I understood str_has_prefix() right,
str_has_prefix("DUMB-MODE", "D")
> > == strlen("DUMB-MODE") would return true, while it's
actually what we
> > want to avoid.
> 
> That's not true, str_has_prefix("DUMB-MODE", "D")
== strlen("D") is.
> 
> > It's also used indeed in drm_get_tv_mode_from_name(), where we try
to
> > match a list of names with one passed as argument.
> >
> > With drm_get_tv_mode_from_name("NSTC",
strlen("NTSC")), we would end up
> > calling str_has_prefix("NTSC-J", "NTSC") ==
strlen("NTSC-J") which would
> > work. However, we end up calling prefix not a prefix, but an entire
> > string we want to match against, which is very confusing to me too.
> 
> If I get this right, you have a string and you want to check if that has
> a certain prefix. Additionally, you want to check the prefix is a
> certain length.
> 
> Sure, that the prefix is a certain length is more of a property of the
> string, which is NUL terminated later than at length, but that's
doesn't
> really matter.
> 
> That condition is simply str_has_prefix(string, prefix) == length.
Ack. I'm ok with the implementation being done that way, but I'd really
prefer to still have some macro to make the name less confusing. Would
that work for you? What name would be better in your opinion?

Thanks!
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220907/9cca6fa7/attachment-0001.sig>