Another simple proposal: - Remove params() from the API (with deprecation) - Update objects that can easily be updated and remove the params() calls/casts. I think this is most cases of the params use outside of the constructor. - Don't worry about the others that use the params() function in many places that require deeper updates - Update the documentation to say that the "best practice" is to not use the param function - In the future, make sure that new objects don't store the params object.
Cheers, Jason On Thu, Oct 22, 2020 at 8:51 AM Giacomo Travaglini < [email protected]> wrote: > Let me add to the plate a simple proposal though still a bit verbose and > probably not that different from a manual cast > > > > Defining a template method in SimObject: > > > > *template <class Obj>* > > *Obj params()* > > > *{ return static_cast<Obj>(_params);* > > * Or more secure:* > > * param = dynamic_cast<Obj>(_params);* > > * assert(param);* > > * return param;* > > *}* > > > > And delegate the type specification on the client side: > > > > For example in my shiny new > > > > Class MyObject : public SimObject {} > > > > I would use > > > > auto p = params<MyObjectParams*>(); > > > > You still have to type your param type so that doesn’t make it the best > but I believe it’s the simplest one > > > > Giacomo > > > > *From:* Jason Lowe-Power via gem5-dev <[email protected]> > *Sent:* 22 October 2020 16:26 > *To:* gem5 Developer List <[email protected]> > *Cc:* Gabe Black <[email protected]>; Jason Lowe-Power < > [email protected]> > *Subject:* [gem5-dev] Re: SimObject params() method > > > > 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 > > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. >
_______________________________________________ gem5-dev mailing list -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
