That would work, but we don't have includes in the middle of a class like
that anywhere else in gem5 (that I can remember at least), which I think is
for the best :-).
Gabe
On Thu, Oct 22, 2020 at 6:28 PM Gambord, Ryan via gem5-dev <
gem5-dev@gem5.org> wrote:
> @Gabe Black <gabebl...@google.com>
> Here is an example of what this looks like. It requires slightly modifying
> the boilerplate code generated for params structs, and then moving the
> include from the top of the source file to inside the class. This brings
> the struct into the class namespace.
>
> This should work fine as long as pybind can work with a FQN of an object.
>
> MySimObject.hh
> #ifndef __MYSIMOBJECT_HH__
> #define __MYSIMOBJECT_HH__
>
> class MySimObject
> {
> public:
> #include "MySimObjectParams.hh"
>
> public:
> MySimObject(Params * p) : myint(p->myint) {}
>
> int & myInt() { return myint; }
>
> private:
> int myint;
> };
> #endif // __MYSIMOBJECT_HH__
>
> MySimObjectParams.hh
> // Generated automatically by SCons //
>
> struct Params {
> int myint;
> };
>
> main.cc
> #include <iostream>
> #include <cstdlib>
>
> #include "MySimObject.hh"
>
> int
> main( int argc, char *argv[]){
> MySimObject::Params p;
> p.myint = 5;
> MySimObject my_so_instance(&p);
> std::cout << my_so_instance.myInt() << std::endl;
> return EXIT_SUCCESS;
> }
>
> Ryan Gambord
> <gambo...@oregonstate.edu>
>
>
>
> On Thu, Oct 22, 2020 at 5:20 PM Gabe Black via gem5-dev <gem5-dev@gem5.org>
> wrote:
>
>> [This email originated from outside of OSU. Use caution with links and
>> attachments.]
>> I definitely agree that we need to use more namespaces and be better
>> citizens in the global namespace.
>>
>> See https://gem5.atlassian.net/browse/GEM5-730
>>
>> Mechanically, I don't think we can define something like
>> Mem::Ruby::Network::BasicLink::Params since that would require adding a
>> Params type to the not yet defined BasicLink class. You can do that with
>> namespaces, but not with classes. We also couldn't add a params function to
>> the SimObject for the same reason. Python doesn't define the SimObject
>> class at all, even though it might look like it does because the python
>> objects and C++ objects often (but not always) share the same name. The
>> Python objects actually are the Params structs in C++, and then python uses
>> the create method to make the actual SimObjects. The real C++ SimObject
>> classes have no python analogue, although python has pointers to them and
>> can indirectly call methods on them once they've been instantiated. That's
>> how callbacks like init(), startup(), etc are handled.
>>
>> Gabe
>>
>> On Thu, Oct 22, 2020 at 10:25 AM Gambord, Ryan via gem5-dev <
>> gem5-dev@gem5.org> wrote:
>>
>>> 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
>>> <gambo...@oregonstate.edu>
>>>
>>>
>>>
>>> On Thu, Oct 22, 2020 at 9:18 AM Jason Lowe-Power via gem5-dev <
>>> gem5-dev@gem5.org> 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 <
>>>> giacomo.travagl...@arm.com> 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 <gem5-dev@gem5.org>
>>>>> *Sent:* 22 October 2020 16:26
>>>>> *To:* gem5 Developer List <gem5-dev@gem5.org>
>>>>> *Cc:* Gabe Black <gabe.bl...@gmail.com>; Jason Lowe-Power <
>>>>> ja...@lowepower.com>
>>>>> *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 <
>>>>> gem5-dev@gem5.org> 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 -- gem5-dev@gem5.org
>>>>> To unsubscribe send an email to gem5-dev-le...@gem5.org
>>>>> %(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 -- gem5-dev@gem5.org
>>>> To unsubscribe send an email to gem5-dev-le...@gem5.org
>>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>>
>>> _______________________________________________
>>> gem5-dev mailing list -- gem5-dev@gem5.org
>>> To unsubscribe send an email to gem5-dev-le...@gem5.org
>>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>>
>> _______________________________________________
>> gem5-dev mailing list -- gem5-dev@gem5.org
>> To unsubscribe send an email to gem5-dev-le...@gem5.org
>> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>
> _______________________________________________
> gem5-dev mailing list -- gem5-dev@gem5.org
> To unsubscribe send an email to gem5-dev-le...@gem5.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
_______________________________________________
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
