On Tue, 2025-08-26 at 21:05 +0300, Costa Shulyupin wrote:
> On Thu, 21 Aug 2025 at 06:58, Crystal Wood <crw...@redhat.com> wrote:
> > The hist members were very similar between timerlat and top, so
> > just use one common hist struct.
> ...
> > @@ -27,4 +38,10 @@ struct common_params {
> ...
> > + struct hist_params hist;
>
> Moving `hist_params` into `common_params` could lead to
> the `common_params` struct becoming a "god object" antipattern.
> The `common_params` struct is intended to be tool-agnostic.
>
> I suggest we use `hist_params` within the tools themselves,
> rather than in `common_params`.
Seems like that would get in the way of further consolidation of hist
code... and then you could just as well complain that the struct is
present when using one of the top tools, unless you separate *those*
out.
Just because someone someone came up with a name and called it
an "antipattern" doesn't mean there's always a benefit (relative to
cost) to breaking things up as much as you can. It was enough of a
nuisance as is to deal with separating timerlat_params, and now we've
got "common." littered all over the place (though sometimes that
indicates a good candidate for future code consolidation).
At this point I'm almost wishing I'd just made them all global variables
:-)
Keep in mind that we're refactoring existing code that is not designed
in a particularly object-oriented manner. This patchset certainly
doesn't reflect every cleanup that I'd like to see in this code, but I
think it's an improvement. There's plenty of room for followup patches.
-Crystal
