As I look at this more, I do see a few places where the params struct is set aside for later legitimately. There aren't tons of them, but I think it's muddy enough where I don't want to go in with a hack saw quite yet.
Gabe On Thu, Oct 22, 2020 at 5:10 PM Gabe Black <[email protected]> wrote: > Ok, I'll make Params typedefs and params() usage targets of opportunity. > > Gabe > > On Thu, Oct 22, 2020 at 8:26 AM Jason Lowe-Power via gem5-dev < > [email protected]> wrote: > >> Hey Gabe, >> >> Thanks for bringing this up. I have also been bothered by the lack of >> consistency with how params are used. I can't think of an example of when >> you need to store the params object. I would be all for getting rid of the >> params() function and updating the documentation to say that it's best >> practice to *not* save the params struct after the constructor. If some >> object had a good reason to go against this best practice, that would be >> OK, and we wouldn't need to enforce any specific design or pattern on these >> exceptions. I would prefer to remove the params() function than add more >> template magic. >> >> On Thu, Oct 22, 2020 at 1:18 AM Gabe Black via gem5-dev < >> [email protected]> wrote: >> >>> Hi folks. I'm looking at the SimObject header, and I see a few things in >>> there which are marked as part of the API and maybe shouldn't be. >>> Specifically I'm talking about the Params typedef, and the params() method. >>> There is also the _params member variable which I can see being a part of >>> the API since it can be used by other classes to make their own params() >>> function (more on that below), but the Params typedef is more or less an >>> implementation detail, and the params() method is essentially worthless >>> because it returns a SimObjectParams which doesn't have anything except the >>> object's name in it, and you can already get that with the name() method. >>> >> >> I agree. I think the typedef is useless and shouldn't be in the API. It's >> unfortunate that the API proposals didn't get more reviews. I think it's >> probably OK to just drop that from the API, but the params() function >> itself is going to need to be deprecated. >> >> >>> >>> *Params pattern* >>> >>> This gets to the whole Params params() pattern which is sporadically >>> implemented in some SimObjects in gem5. This pattern is that a given >>> SimObject subclass will define a Params typedef which corresponds to its >>> Params struct type, and then also define a params() method which returns >>> the _params from SimObject cast up to that type. >>> >>> The Params typedef itself primarily makes the definition of the >>> constructor a little more legible, since the FullFooTypeForTheArmISAParams >>> can be really verbose. >>> >> >> I think verbose is fine. I would vote to abolish all params typedefs. >> >> >>> >>> Storing the params struct itself could theoretically serve the purpose >>> of having a bunch of parameters and not having to create a member variable >>> for each one, spend a bunch of text copying values over in the constructor, >>> etc. I think most of the time this is unnecessary, but if an object has >>> tons of values in it for some reason this could make sense. >>> >> >> I don't think there are any examples of this in the codebase. I think in >> all cases the params data is copied into member variables. If there are >> cases where data isn't copied, I doubt it was with a strong reason in mind. >> The one exception to this might be Ruby, but it's an exception for all the >> wrong reasons ;). >> >> >>> >>> The params() method then makes the _params member available with the >>> appropriate type, so that all the FooParams members are accessible. It also >>> makes the params struct accessible outside the object which is used in a >>> place or two to read parameters without there needing to be a member in the >>> object, and probably some sort of accessor to read it. >>> >>> There are two main problems with this system. First, when used, it adds >>> a small but not trivial amount of boilerplate to any given class. Second, >>> it's very sporadically implemented. I think in a lot of places it's there >>> just because people aren't sure what it's for or if they need it, so they >>> just put one there regardless. I think I've done that in the past. >>> >>> *Alternative* >>> >>> The existence of the Params type and the params() method could be >>> partially eliminated by defining a templated params() method which took a >>> SimObject reference and/or pointer as its first parameter. It could then >>> figure out what Params struct went with that SimObject type using >>> typedef/template magic set up by the Params building code, and then perform >>> the appropriate cast. >>> >>> This has three downsides, two minor and one more serious. The minor one >>> is that when a class uses this method internally, it would have to do >>> something like params(this) instead of just params(). That's a fairly minor >>> difference and not that big a deal. For external consumers that would be >>> less of a problem since it would change from foo->params() to params(foo). >>> >>> The second minor issue is that the name params() is very short, and >>> likely to collide with other names. We could define that with SimObject as >>> a static method, but then that would make foo->params() turn into the more >>> verbose SimObject::params(foo), or (and I haven't checked if this is legal >>> syntax) the more odd looking foo->params(foo). The params() class can't be >>> a non-static method, because then the type of "this" would be fixed by >>> where it was defined, meaning it would not cast _params to the right type. >>> I was not able to find any mechanism in c++ that would let you treat "this" >>> as an argument for template type resolution. >>> >>> The third more serious problem is that this implicitly breaks the >>> ability to use two different SimObject types in python to represent the >>> same SimObject type in C++. I don't know if this happens in practice, and >>> it's also broken by the original Params pattern, since there can be only >>> one typedef in a given class. Since Params is applied adhoc manually, >>> something that is generally not good, it actually avoids this problem by >>> just not existing anywhere that would break that assumption. >>> >>> *Other option* >>> >>> Another option would be to have a templated class which would define a >>> Params type and a params() method, and inherit that into each SimObject >>> which wants to have those members. It would itself inherit from its >>> parameter which would keep the inheritance hierarchy intact, and make it >>> possible to override Params and params from super classes: >>> >>> FooObject : public ParamsMembers<SimObject> >>> >>> This has a similar problem to the above if exactly what Params type to >>> use is automatic, although here it could be an additional template >>> argument. This also trades some boilerplate for less boilerplate, has to be >>> applied manually to any classes that want to take advantage of it, and >>> obfuscates the definition of those classes. >>> >>> Gabe >>> _______________________________________________ >>> gem5-dev mailing list -- [email protected] >>> To unsubscribe send an email to [email protected] >>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s >> >> _______________________________________________ >> gem5-dev mailing list -- [email protected] >> To unsubscribe send an email to [email protected] >> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > >
_______________________________________________ gem5-dev mailing list -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
