Em Fri, 1 Aug 2025 07:28:41 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> escreveu:
> Em Thu, 31 Jul 2025 18:13:18 -0600
> Jonathan Corbet <corbet@xxxxxxx> escreveu:
>
> > dump_struct is one of the longest functions in the kdoc_parser class,
> > making it hard to read and reason about. Move the definition of the prefix
> > transformations out of the function, join them with the definition of
> > "attribute" (which was defined at the top of the file but only used here),
> > and reformat the code slightly for shorter line widths.
> >
> > Just code movement in the end.
>
> This patch itself LGTM:
>
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
In time, my R-B from patch 4 and above assumes that patch 3 is
dropped, as I'm not re-checking the regular expressions.
>
> but see my notes below:
>
> > +struct_prefixes = [
> > + # Strip attributes
> > + (struct_attribute, ' '),
> > + (KernRe(r'\s*__aligned\s*\([^;]*\)', re.S), ' '),
> > + (KernRe(r'\s*__counted_by\s*\([^;]*\)', re.S), ' '),
> > + (KernRe(r'\s*__counted_by_(le|be)\s*\([^;]*\)', re.S), ' '),
> > + (KernRe(r'\s*__packed\s*', re.S), ' '),
> > + (KernRe(r'\s*CRYPTO_MINALIGN_ATTR', re.S), ' '),
> > + (KernRe(r'\s*____cacheline_aligned_in_smp', re.S), ' '),
> > + (KernRe(r'\s*____cacheline_aligned', re.S), ' '),
> > + #
> > + # Unwrap struct_group macros based on this definition:
> > + # __struct_group(TAG, NAME, ATTRS, MEMBERS...)
> > + # which has variants like: struct_group(NAME, MEMBERS...)
> > + # Only MEMBERS arguments require documentation.
> > + #
> > + # Parsing them happens on two steps:
> > + #
> > + # 1. drop struct group arguments that aren't at MEMBERS,
> > + # storing them as STRUCT_GROUP(MEMBERS)
> > + #
> > + # 2. remove STRUCT_GROUP() ancillary macro.
> > + #
> > + # The original logic used to remove STRUCT_GROUP() using an
> > + # advanced regex:
> > + #
> > + # \bSTRUCT_GROUP(\(((?:(?>[^)(]+)|(?1))*)\))[^;]*;
> > + #
> > + # with two patterns that are incompatible with
> > + # Python re module, as it has:
> > + #
> > + # - a recursive pattern: (?1)
> > + # - an atomic grouping: (?>...)
> > + #
> > + # I tried a simpler version: but it didn't work either:
> > + # \bSTRUCT_GROUP\(([^\)]+)\)[^;]*;
> > + #
> > + # As it doesn't properly match the end parenthesis on some cases.
> > + #
> > + # So, a better solution was crafted: there's now a NestedMatch
> > + # class that ensures that delimiters after a search are properly
> > + # matched. So, the implementation to drop STRUCT_GROUP() will be
> > + # handled in separate.
> > + #
> > + (KernRe(r'\bstruct_group\s*\(([^,]*,)', re.S), r'STRUCT_GROUP('),
> > + (KernRe(r'\bstruct_group_attr\s*\(([^,]*,){2}', re.S), r'STRUCT_GROUP('),
> > + (KernRe(r'\bstruct_group_tagged\s*\(([^,]*),([^,]*),', re.S), r'struct \1 \2; STRUCT_GROUP('),
> > + (KernRe(r'\b__struct_group\s*\(([^,]*,){3}', re.S), r'STRUCT_GROUP('),
> > + #
> > + # Replace macros
> > + #
> > + # TODO: use NestedMatch for FOO($1, $2, ...) matches
>
> This comment is actually related to patch 03/12: regex cleanups:
>
> If you want to simplify a lot the regular expressions here, the best
> is to take a look at the NestedMatch class and improve it. There are lots
> of regular expressions here that are very complex because they try
> to ensure that something like these:
>
> 1. function(<arg1>)
> 2. function(<arg1>, <arg2>,<arg3>,...)
>
> are properly parsed[1], but if we turn it into something that handle (2) as
> well, we could use it like:
>
> match = NestedMatch.search("function", string)
> # or, alternatively:
> # match = NestedMatch.search("function($1, $2, $3)", string)
>
> if match:
> arg1 = match.group(1)
> arg2 = match.group(2)
> arg3 = match.group(3)
>
> or even do more complex changes like:
>
> NestedMatch.sub("foo($1, $2)", "new_name($2)", string)
>
> A class implementing that will help to transform all sorts of functions
> and simplify the more complex regexes on kernel-doc. Doing that will
> very likely simplify a lot the struct_prefixes, replacing it by something
> a lot more easier to understand:
>
> # Nice and simpler set of replacement rules
> struct_nested_matches = [
> ("__aligned", ""),
> ("__counted_by", ""),
> ("__counted_by_(be|le)", ""),
> ...
> # Picked those from stddef.h macro replacement rules
> ("struct_group(NAME, MEMBERS...)", "__struct_group(, NAME, , MEMBERS)"),
> ("struct_group(TAG, NAME, ATTRS, MEMBERS...)",
> """ __struct_group(TAG, NAME, ATTRS, MEMBERS...)
> union {
> struct { MEMBERS } ATTRS;
> struct __struct_group_tag(TAG) { MEMBERS } ATTRS NAME;
> } ATTRS"""),
> ...
> ]
>
> members = trim_private_members(members)
> for from, to in struct_nested_matches:
> members = NestedMatch.sub(from, to, members)
>
> Granted, wiring this up takes some time and lots of testing - we should
> likely have some unit tests to catch issues there - but IMO it is
> worth the effort.
>
> -
>
> [1] NestedMatch() is currently limited to match function(<args>), as it was
> written to replace really complex regular expressions with
> recursive patterns and atomic grouping, that were used only to
> capture macro calls for:
>
> STRUCT_GROUP(...)
>
> I might have used instead "import regex", but I didn't want to add the
> extra dependency of a non-standard Python library at the Kernel build.
>
> Thanks,
> Mauro
Thanks,
Mauro
