Make Coefficient `Eval` const + Other `const`-Related Updates for Coefficients [constcoeff-dev]
In the context of #4171, it doesn't make sense for Eval (and Project) to not have const qualifiers, as calls to them should not be altering the state of the coefficient objects (except for the Coefficient::Eval(...., double time), where a SetTime is called prior to Eval).
Because Eval is not constant, there are a variety of situations where a coefficient must be passed as a non-const argument to a function that should not change the state of the coefficient at all, like GridFunction::Project_____ and to linear form and bilinear form integrator constructors. This makes it difficult if one wants to pass a coefficient as const to a class such as an operator that may use it but does not change it.
This PR makes Eval, Project, and a variety of other member functions (such as PWConstCoefficient::GetNConst) const that should be const. Additionally, noted in the commit message for af29df2b4d302bc891646ccd833cad12230fcfff , some member functions that are declared as constant allowed access to non-const member variable references - this PR fixes that as well, which may be a breaking change.
Lastly, VectorSumCoefficient posed some troubles in implementing this as the Vector and double member variables are updated in Eval. More details are explained in the commit message for 841a546bfa9290a20e160ba073446e207a454eff, but the current PR simply just sets these member variables as mutable. Punchline is that it's not the cleanest solution, but an alternative approach would be to use separate auxiliary variables in Eval which may lead to a loss in backward compatibility.
Because it is very common in codes using MFEM to define a custom coefficient, the non-const Eval is kept in to maintain backwards compatibility. The ultimate goal after this PR would be to update existing functions in the code that call Eval (and/or Project) to accept const coefficient types only to ensure that the Eval(...) const is called (or if it is preferable, I can return this PR to draft and include that as well).
Thanks for the contribution! 👍 I wanted to do that a long time ago, but have not found time for that. So in general, I am in favor of the change, but I have multiple points/remarks here:
- We should think about that
Eval(..., double t)variant, because it is also like evaluation at a point, but in space-time in this case, so it should be const as well and the permanent change of the internal time inCoefficientis more like a side effect. However, it might be the case that some existing code relies on this side effect, so we would break backward compatibility if we change the time just temporarily to remain true to the constness of the Coefficient 😕 Maybe there could remain a non-const version, which would have this side effect for this compatibility reason 🤔 . - Consider using
MFEM_DEPRECATEDmacro, which will reveal what has not been updated yet. However, we should think about it before merging, because it will annoy many users and the change is not that important to make them angry 😅 . It is just a decision of the compiler to default to the non-const version when you have a non-const object, so one might be innocently in that situation. - I do not like that getters (like
GetACoeff()), which return const objects now. The getters should return what the class has obtained, unless there is a good reason for that. So either change the constructors to take const coefficients, or keep getters returning non-const objects. I can imagine these getters are used somewhere to change the coefficients somehow, so I would keep backward compatibility here and use thatMFEM_DEPRECATED.