Closed mortezah closed 3 years ago
@cwsmith I have started the process of hiding the adapt options. I was able to make it work without exposing too many setters in the ma::Input
class until I got tophasta
. Apparently, a lot of options are set there manually, which would require me to implement setters for all the adapt options (which sort of defeats the purpose of this pull request). I am going to give this some thought and see if I can do the same trick of using friend functions, specifically for `phasta' as a work-around.
The phasta preprocessor, chef
, is principally used as an executable with many runtime options.
Hmm. It looks like ma::input
mostly appears in phAdapt.cc
and once in ph_convert.cc
. In phAdapt.cc
the big offender is AdaptCallback
.
What if the setupAdvanced
friend function (which can only be defined once per binary, iiuc) was replaced, or supplemented by, a friend class AdvancedInput
that the user can define with multiple functions that directly access the input class? This would seemingly solve the phasta problem of needing to set/get the input vars from several different functions.
@mortezah what's the end goal? I think current C++ best practices is to use public members for things like options which have trivial setters/getters. Instead of having the validateInput
function you do the validation for each setting in its setter which would make the input always have a valid state.
Is there a reason that configure
/setDefaultValues
aren't just the constructors for Input
?
Also, having this many friend functions is probably not a great idea. I think since you are already doing breaking changes this class could be significantly improved.
@jacobmerson the goal was to find a quick solution to hide the mesh adapt options from the basic users, but have a way for advanced users to be able to set those. Hence the reason for making setAdnvacedInputOption a friend of the input class.
I agree that things like configure
can become simple members of the input class. But in the first pass at this, I didn't want to bother about that.
I just skimmed what you have, so I may be missing something, but I think a better option may be to give the Input
class a constructor and you can set up the default options in the constructor. If you want to verify options as they are set you have setters/getters for each option. If you keep the verify
function then all your options are just public data members.
So essentially instead of having configure(mesh,...)
you have the constructor Input(mesh,...)
.
@jacobmerson I get the point about constructor and verify. But again I do not want to make a lot of changes to the code at this point. ma::Input
is not the only class that does not follow standard C++ practice.
Also, the starting point behind these changes was to not give the users the ability to modify these options (at least not in an obvious way). So having data members public or having individual setter member functions will defeat that purpose.
I might have a better solution for this which follows proper c++ standards. I'll update you guys once that's in place.
@mortezah I like the direction, but I have one question...Do any of the ma algorithms need to take a non-advanced Input file and mutate it?
If the answer is no, you can use a similar approach without the boiler plate. Essentially instead of using two separate classes we can just use const-correctness to force novice users to not be able to modify the input options.
So, the prototypes of your free functions look like this:
// basic configuration returns a pointer to const Input* so user cannot modify
const Input* configure(Mesh* m, ...){ return new Input(m, ...); }
// advanced function just does
Input* configureAdvanced(Mesh* m, ...){ return new Input(m, ...); }
With the approach, we only need the one class, and there is no need for checking modifiable anywhere. Also there is a slight performance win and a large usability win because const correctness is checked at compile time. So, better performance because we don't have to check modifiable and no virtual functions. Also, compile time errors >> runtime errors.
If we want a really strong guarantee that the basic user isn't modifying any parameters we can do the following:
class Input {
public:
Input* clone(Input* in) const { return new Input(*in);}
const Input* clone(const Input* in) const { return new Input(*in);}
// friend the free creation functions as necessary
private:
// private copy constructor
Input(const Input& in);
};
Now if the user gets a const Input*
they have no way to modify anything and we provide the function
Input* makeAdvanced(const Input*);
@mortezah I like the direction, but I have one question...Do any of the ma algorithms need to take a non-advanced Input file and mutate it?
If the answer is no, you can use a similar approach without the boiler plate. Essentially instead of using two separate classes we can just use const-correctness to force novice users to not be able to modify the input options.
So, the prototypes of your free functions look like this:
// basic configuration returns a pointer to const Input* so user cannot modify const Input* configure(Mesh* m, ...){ return new Input(m, ...); } // advanced function just does Input* configureAdvanced(Mesh* m, ...){ return new Input(m, ...); }
With the approach, we only need the one class, and there is no need for checking modifiable anywhere. Also there is a slight performance win and a large usability win because const correctness is checked at compile time. So, better performance because we don't have to check modifiable and no virtual functions. Also, compile time errors >> runtime errors.
If we want a really strong guarantee that the basic user isn't modifying any parameters we can do the following:
class Input { public: Input* clone(Input* in) const { return new Input(*in);} const Input* clone(const Input* in) const { return new Input(*in);} // friend the free creation functions as necessary private: // private copy constructor Input(const Input& in); };
Now if the user gets a
const Input*
they have no way to modify anything and we provide the functionInput* makeAdvanced(const Input*);
This might actually work. I guess all I will need to do is to make the const input object an advanced (non-const) one as soon as I get inside the highest level mesh adapt routine (which takes an Input
as an argument), and that way everything would be modifiable inside mesh adapt.
I guess I can also apply the same trick to the current implementation (where everything is public and there are no getters and setters). That way the amount of change to the code would be a lot less. Obviously, then things would not be following C++ coding standards.
@cwsmith and @jacobmerson do you have a preference between the two approaches
I think having the public members variables is probably the nicest solution because all the setters and getters are trivial and essentially just boilerplate. And it requires the least changes for any downstream code…all they need to do is a one line change to get the non-const input object. But, I think either is a fine choice and is essentially just stylistic preference.
— Jacob Merson
On Jul 4, 2021, at 10:20 AM, Morteza @.***> wrote:
@mortezah I like the direction, but I have one question...Do any of the ma algorithms need to take a non-advanced Input file and mutate it?
If the answer is no, you can use a similar approach without the boiler plate. Essentially instead of using two separate classes we can just use const-correctness to force novice users to not be able to modify the input options.
So, the prototypes of your free functions look like this:
// basic configuration returns a pointer to const Input so user cannot modify const Input configure(Mesh* m, ...){ return new Input(m, ...); }
// advanced function just does Input configureAdvanced(Mesh m, ...){ return new Input(m, ...); } With the approach, we only need the one class, and there is no need for checking modifiable anywhere. Also there is a slight performance win and a large usability win because const correctness is checked at compile time. So, better performance because we don't have to check modifiable and no virtual functions. Also, compile time errors >> runtime errors.
If we want a really strong guarantee that the basic user isn't modifying any parameters we can do the following:
class Input { public: Input clone(Input in) const { return new Input(in);} const Input clone(const Input in) const { return new Input(in);}
// friend the free creation functions as necessary private: // private copy constructor Input(const Input& in); }; Now if the user gets a const Input* they have no way to modify anything and we provide the function
Input makeAdvanced(const Input); This might actually work. I guess all I will need to do is to make the const input object an advanced (non-const) one as soon as I get inside the highest level mesh adapt routine (which takes an Input as an argument), and that way everything would be modifiable inside mesh adapt.
I guess I can also apply the same trick to the current implementation (where everything is public and there are no getters and setters). That way the amount of change to the code would be a lot less. Obviously, then things would not be following C++ coding standards.
@cwsmith and @jacobmerson do you have a preference between the two approaches
having getters and setters with private options in the input class, along with constant correctness suggested by @jacobmerson keeping the input class as it is in the current code, with public options but adding constant correctness. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
I closed this one and will open a new one where nothing much has changed except the constant configure output.
Here is the new pull request https://github.com/SCOREC/core/pull/344
This is achieved by having a base
ma::Input
class with setters and getters. The setters will be implemented in a way that they would print an error and abort. There will be two subclassesma::InputBasic : public ma::Input
with no extra additionsma::InputAdvanced : public ma::Input
where the setters will be properly implemented to set the corresponding properties of the base classHaving the above, users can configure mesh adapt in two different ways
ma::Input* in = ma::configure(...)
, which returns an input object of the typema::InputBasic
and therefore the user will not be able to make any changes.ma::Input* in = ma::configureAdvanced(...)
, which returns an input object of the typema::InputAdvanced
, which can be modified by the users if they chose to do so.Update: Switched to constant-correctness and only 1
Input
class. The defaultconfigure
APIs return a constant pointer that cannot be modified. There is an additional APImakeAdvanced
that takes a constantInput
and removes its constant-ness for advanced users if they need to modify the input object before the call to mesh_adapt.ma::configure()
APIs.