Also that (like the other mechanisms) means that there can be at most one
C++ class for a given Params class. It would be impossible to have
FooParams -> SimObj and BarParams -> SimObj since there is only room for
one Params in a class.
That said, we could turn the FooParams and BarParams into Params::Foo and
Params::Bar. That would be fairly trivial, and would mean we'd only have
one name to dodge, Params, although that's an easier name to collide with
accidentally.
I think a step in the right direction would be to start putting all our
gem5 stuff into a gem5 namespace. Namespaces are easy to fold away in
context (using, opening the namespace and working within it), and then we
would only have to worry about internal collisions which I think are a lot
easier to manage.
We could even start doing that now without breaking compatibility by
putting things in a gem5 namespace and then putting a "using namespace
::gem5;" at the end to maintain compatibility until we decide to switch.
Gabe
On Thu, Oct 22, 2020 at 7:50 PM Gabe Black <gabe.bl...@gmail.com> wrote:
> 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
