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

[Gabriele Monaco <gmonaco@redhat.com>]

On Mon, 2025-07-21 at 16:38 +0200, Nam Cao wrote:
> 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.
> > 
> > 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

Well, all the python best practices I follow are the ones reported by
pylint, I guess I need to configure it to follow those.
Thanks for pointing that out!

> 
> >          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.

Good point.

Thanks,
Gabriele