[Einstein Toolkit] #875: Cactus should define 'restrict', with a value dependent on autoconf information
#875: Cactus should define 'restrict', with a value dependent on autoconf information -------------------------+-------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Keywords: | -------------------------+-------------------------------------------------- The Kranc ticket https://github.com/ianhinder/Kranc/issues/62 mentions arguments why to do this: to make certain optimizations possible. While 'restrict' is not a C keyword, many compilers support it, leading to measurable speedup in cases. At the moment Cactus only defines CCTK_RESTRICT, but it has been commented that using this leads to ugly code. Defining 'restrict' would beautify this, as well as make it easier to directly use code which relies on 'restrict' being present (when this isn't actually the case, or is called differently).
A counter argument would be that this would effectively introduce a keyword 'restrict' to Cactus. Code using this name already for something else would need to be changed. Given the positive effects of this change I would say that this outweights this potential problem.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
I strongly support this.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by rhaas):
The only uglyness I can see is that an editors syntax highlighting does not work with CCTK_RESTRICT (same as CCTK_INLINE or the CCTK_XXX types). FOr this a better solution might be if users change their editors syntax highlighting.
Generally I dislike redifining language keywords (I would expect restrict to be a keyword in C99) since it means that one has to know that someone changes the language.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by eschnett):
Roland: Generally, I agree with you, but specifically for restrict, I don't. What do you think about restrict in particular?
For example: - Most C++ compilers have an option to make "restrict" a keyword - "restrict" may become part of the next C++ standard (I think I read this somewhere, but now can't find a pointer) - Since Carpet already #defines restrict, we know it is safe to use everywhere
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
And Carpet is the most likely thorn to have some other meaning of the word "restrict".
The uglyness that I see is that CCTK_RESTRICT is longer and is in capital letters. We don't program in capital letters most of the time, and the CCTK_ prefix is a namespacing hack.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: new => review
Comment:
I propose this patch:
{{{ Index: cctk_Config.h.in =================================================================== --- cctk_Config.h.in (revision 4811) +++ cctk_Config.h.in (working copy) @@ -381,14 +381,10 @@ /* Some C++ compilers recognise the restrict keyword */ #undef HAVE_CCTK_CXX_RESTRICT #undef CCTK_CXX_RESTRICT - -/* Since this is non-standard leave commented out for the moment */ -#if 0 /* Define to empty if the keyword does not work. */ #ifdef CCTK_CXX_RESTRICT # define restrict CCTK_CXX_RESTRICT #endif -#endif
/* Allow the use of CCTK_RESTRICT as a qualifier always. */ #ifdef CCTK_CXX_RESTRICT }}}
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reviewed_ok Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by knarf):
* status: review => reviewed_ok
Comment:
Please apply.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Applied.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: --------------------------+-------------------------------------------------
Comment (by hinder):
Can this be made to work for CUDA as well?
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: reopened Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: closed => reopened * resolution: fixed =>
Comment:
Can you open a separate ticket to add autoconf magic for CUDA? We currently only distinguish between C and C++.
#875: Cactus should define 'restrict', with a value dependent on autoconf information --------------------------+------------------------------------------------- Reporter: knarf | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: Cactus | Version: development version Resolution: fixed | Keywords: --------------------------+------------------------------------------------- Changes (by eschnett):
* status: reopened => closed * resolution: => fixed
Comment:
The CUDA ticket has been opened as #891.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit