Re: [PATCH v2 14/41] drm/modes: Move named modes parsing to a separate function

[Maxime Ripard]

Hi,

On Tue, Aug 30, 2022 at 04:36:23PM +0300, Jani Nikula wrote:
> On Tue, 30 Aug 2022, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > Hi,
> >
> > On Tue, Aug 30, 2022 at 01:43:07PM +0300, Jani Nikula wrote:
> >> On Tue, 30 Aug 2022, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> >> > On Mon, Aug 29, 2022 at 3:13 PM Maxime Ripard <maxime@xxxxxxxxxx> 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

Attachment: signature.asc
Description: PGP signature