Re: [PATCH v4 08/14] verification/rvgen: Organise Kconfig entries for nested monitors

[Nam Cao <namcao@linutronix.de>]

On Mon, Jul 21, 2025 at 10:23:18AM +0200, Gabriele Monaco wrote:
> The current behaviour of rvgen when running with the -a option is to
> append the necessary lines at the end of the configuration for Kconfig,
> Makefile and tracepoints.
> This is not always the desired behaviour in case of nested monitors:
> while tracepoints are not affected by nesting and the Makefile's only
> requirement is that the parent monitor is built before its children, in
> the Kconfig it is better to have children defined right after their
> parent, otherwise the result has wrong indentation:
> 
> [*]   foo_parent monitor
> [*]     foo_child1 monitor
> [*]     foo_child2 monitor
> [*]   bar_parent monitor
> [*]     bar_child1 monitor
> [*]     bar_child2 monitor
> [*]   foo_child3 monitor
> [*]   foo_child4 monitor
> 
> Adapt rvgen to look for a different marker for nested monitors in the
> Kconfig file and append the line right after the last sibling, instead
> of the last monitor.
> Also add the marker when creating a new parent monitor.
> 
> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>

Some nitpicks below. But regardless:
Reviewed-by: Nam Cao <namcao@linutronix.de>

> -    def __patch_file(self, file, marker, line):
> +    def _patch_file(self, file, marker, line):
> +        assert(self.auto_patch)

Nit: follows PEP8 unless there is a reason not to: assert self.auto_patch

>          file_to_patch = os.path.join(self.rv_dir, file)
>          content = self._read_file(file_to_patch)
>          content = content.replace(marker, line + "\n" + marker)
> @@ -146,7 +147,7 @@ class RVGenerator:
>      def fill_tracepoint_tooltip(self):
>          monitor_class_type = self.fill_monitor_class_type()
>          if self.auto_patch:
> -            self.__patch_file("rv_trace.h",
> +            self._patch_file("rv_trace.h",
>                              "// Add new monitors based on CONFIG_%s here" % monitor_class_type,
>                              "#include <monitors/%s/%s_trace.h>" % (self.name, self.name))
>              return "  - Patching %s/rv_trace.h, double check the result" % self.rv_dir
> @@ -156,10 +157,15 @@ Add this line where other tracepoints are included and %s is defined:
>  #include <monitors/%s/%s_trace.h>
>  """ % (self.rv_dir, monitor_class_type, self.name, self.name)
>  
> +    def _container_marker(self, container = None) -> str:
> +        return "# Add new %smonitors here" % (container + " "
> +                                              if container else "")

PEP8: container=None

And this function name is misleading, it is not marker for only containers.
Perhaps _kconfig_marker() is more appropriate.