#223: Make declaration of CCTK_ARGUMENTS safer --------------------------+------------------------------------------------- Reporter: eschnett | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
This change is a very good idea and should be implemented.
Process:
For the benefit of anyone reading cGH-const.diff, I think the first, second and fourth hunks can be ignored, as they don't seem to be related to this change. I'm not sure what the first hunk is doing, but it is not just a formatting change, so should probably not be committed without being remarked on or put into a separate patch.
Content:
I'm a bit confused about the relation between the summary and comments and the patches. The summary says that cctkGH is made const, whereas it looks to me like the patch makes the local variables which are computed from cctkGH const. My naive interpretation would be that you could still modify variables if you accessed them via cctkGH->... and this seems to be what the CartGrid3D patch is doing. When you say in comment:2 that CartGrid3D and Time need to be modified, are you talking about a further modification to CartGrid3D beyond the current patch, for example one that casts away constness?
The approach taken in the patch looks to me like a good one, and should help to avoid nasty user bugs. Is there a similar patch for the Time thorn, as the summary indicates that this will be needed before these can be committed. Also, for such a pervasive change, running the ET test suite on a couple of architectures might shake out any problems before users running on trunk get bitten by them. Do you want to just commit this and see what falls out, or test it thoroughly before it is committed?