While we're thinking about the params implementations, I'd also like to
propose that we stop cluttering the global namespace with collision-prone
names:
src/.../.../MySimObject.hh
namespace NoClutter {
class MySimObject : ::SimObject
{
#include params/MySimObject.hh
};
};
params/MySimObject.hh
// ... //
struct Params {
// ... //
};
For example, ruby claimed the name "BasicLink", which is pretty generic.
Would be much better if we started moving towards
Mem::Ruby::Network::BasicLink::Params, which would be more idiomatic C++.
The params/xxx.hh sources could even include the boilerplate params()
method into the simobject class if you want. There is *the* canonical
use-case for mid-file includes -- when we have compile-time generated
boilerplate in separate sources.
Right now, gem5 gives me "C with classes" vibes.
Ryan Gambord
<[email protected]>
On Thu, Oct 22, 2020 at 9:18 AM Jason Lowe-Power via gem5-dev <
[email protected]> wrote:
> [This email originated from outside of OSU. Use caution with links and
> attachments.]
> 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
_______________________________________________
gem5-dev mailing list -- [email protected]
To unsubscribe send an email to [email protected]
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
