Cactus: Add CCTK_VINFO, CCTK_VERROR, CCTK_VWARN

Create issue
Issue #1939 closed
Erik Schnetter created an issue

Comments (10)

  1. Roland Haas
    • removed 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 into CCTK_VInfo(CCTK_THORNSTRING, "a %d", 42) ie. one saves on the CCTK_THORNSTRING. The commit thus makes the CCTK_UPPERCASE macros more orthogonal to the corresponding CCTK_LowerCase functions since the difference between the two is now only that one set includes CCTK_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:

    • please provide documentation for the macros in CCTK_Reference.tex
    • there are extra changes that reformat lines that are not otherwise touched by the commit, I would leave those out if possible

    Fine with me as long as the first point is taken care of (the second one is not crucial, but documentation is).

  2. Roland Haas
    • 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.

  3. Ian Hinder
    • changed status to open
    • assigned issue to
    • removed comment

    Related: #2005 is a report of code having been committed that depends on the proposed code in the current ticket which unfortunately breaks the build.

  4. Frank Löffler
    • 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.

  5. Roland Haas
    • 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 over CCTK_Vinfo is along the lines of preferring CCTK_INFO over CCTK_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.

  6. Roland Haas
    • 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.
  7. Log in to comment