- removed comment
Cactus: Add CCTK_VINFO, CCTK_VERROR, CCTK_VWARN
Comments (10)
-
-
- removed comment
This fails to compile code like this for me:
CCTK_VINFO("Message without formatting");
I suspect this could be cured by using
#define CCTK_VINFO(...) CCTK_VInfo(CCTK_THORNSTRING,__VA_ARGS__)
ie only having the VA argument so that the error only shows up if no argument is passed to VINFO at all.
-
- changed status to open
- assigned issue to
- removed comment
Related:
#2005is a report of code having been committed that depends on the proposed code in the current ticket which unfortunately breaks the build. -
- changed status to open
- removed comment
-
- removed comment
Some points: - I don't link other changes in the same diff. It's ok for now, but please don't mix changes even if it just changes upper case / lower case of something unrelated - If I read the changes to the documentation correctly, places with, e.g., CCTK_VInfo are replaced by CCTK_VINFO. Isn't CCTK_VInfo still there, usable, and should stay mentioned, like in "Note that the routines \texttt{CCTK_VError()} and \texttt{CCTK_VWarn()} can only be called from C" - Seeing the diff of the pdf in github mixed with the other changes is annoying. I know this decision was made to keep the diffs to the pdfs small, but it is really annoying here.
-
- removed comment
- If I read the changes to the documentation correctly, places with, e.g., CCTK_VInfo are replaced by CCTK_VINFO. Isn't CCTK_VInfo still there, usable, and should stay mentioned, like in "Note that the routines \texttt{CCTK_VError()} and \texttt{CCTK_VWarn()} can only be called from C"
I noticed that as well and was a bit upset initially. However preferring
CCTK_VINFO
overCCTK_Vinfo
is along the lines of preferringCCTK_INFO
overCCTK_Info
which has always been the case. So my estimate in the review was that this is a valid change that preserves consistency in the documentation and the recommended functions.The pdf diffs are annoying I agree. I don't know of a way on top of my head to avoid them (declaring the pdf's binaries likely will counteract the initial idea of making git create diffs for them in the first place). I don't really see them as any different from the diffs of configure though which is also an auto-generated file and contains lots of changes each time its source file configure.in is changed.
-
- removed comment
(this was stuck in queue and apparently I forgot to hit "submit", sorry).
Please apply.
The commit makes the code build again. Some issues in the documentation remain:
- it replicates the information in the documentation section for CCTK_VWARN in CCTK_VWarn. One should be shortened to refer (with a link) to the other
- the code change mixes new features in one section of the file with code reformatting in another. This makes it hard to review.
-
- changed status to open
- removed comment
-
reporter - changed status to resolved
- removed comment
These change have been applied via commit d05d608484f3b6e74a53f42693efab48343f54e7.
-
- edited description
- changed status to closed
- Log in to comment
As far as I can tell this introduces (among other unrelated changes) a set of macros such that eg.
CCTK_VINFO("a %d", 42)
gets turned intoCCTK_VInfo(CCTK_THORNSTRING, "a %d", 42)
ie. one saves on the CCTK_THORNSTRING. The commit thus makes theCCTK_UPPERCASE
macros more orthogonal to the correspondingCCTK_LowerCase
functions since the difference between the two is now only that one set includesCCTK_THORNSTRING
,__LINE__
,__FILE__
and the other one does not.The patch is lacking documenation updates for the new macros and should not be applied without.
Comments:
Fine with me as long as the first point is taken care of (the second one is not crucial, but documentation is).