Commits

Jed Brown committed 2694ab8

PetscDefined: test whether a configuration macro is defined without #ifdef

This allows testing configuration macros from normal code, thus avoiding
unused variable warnings and allowing the compiler to check for syntax
errors throughout all alternatives. This use C99-style variadic macros.

This trick was introduced by "comex" on Google+ in response to a
question posed by Linus Torvalds.
https://plus.google.com/+LinusTorvalds/posts/9gntjh57dXt

  • Participants
  • Parent commits 59d6dae
  • Branches jed/defined-nocpp

Comments (7)

  1. BarryFSmith

    Huh? But isn't the main use of #ifdef to protect compiles of function calls that simply don't exist otherwise? For example:

    if defined(PETSC_MISSING_LAPACK_GESVD)

        SETERRQ(PETSC_COMM_SELF,PETSC_ERR_SUP,"GESVD - Lapack routine is unavailable.");
    

    else

    if defined(PETSC_USE_COMPLEX)

        PetscStackCallBLAS("LAPACKgesvd",LAPACKgesvd_("A","A",&bell,&bell,&MZa[1+ldMZ],&ldMZ,bcgsl->s,bcgsl->u,&bell,bcgsl->v,&bell,bcgsl->work,&bcgsl->lwork,bcgsl->realwork,&bierr));
    

    else

        PetscStackCallBLAS("LAPACKgesvd",LAPACKgesvd_("A","A",&bell,&bell,&MZa[1+ldMZ],&ldMZ,bcgsl->s,bcgsl->u,&bell,bcgsl->v,&bell,bcgsl->work,&bcgsl->lwork,&bierr));
    

    endif

    endif

    Now, mind you, I would like to completely eliminate the use of CPP in PETSc (while Jed wants to use it in preverse ways) but I don't see what this buys you.

    1. Jed Brown author

      PETSC_USE_DEBUG, PETSC_USE_CTABLE, PETSC_HAVE_HDF5, PETSC_HAVE_MPIIO, PETSC_USE_REAL_SINGLE. Lots of these in mat and dm, for example, often guarding tests that don't need to be protected. Incidentally, gcc and clang, even at -O0 -g elide the references to functions within an if (0) block, so the #else in my patch is not required (but I'd be shocked it that was portable).

      This would get rid of all the PETSC_UNUSED or multiple locations of #ifdef to protect variables that are only used in some branches. It gets syntax checking everywhere. For example, 82be971e00ac63e3249fb9e8441aeb32a33bab6a and b255d4c8f1dd4505b966d7c47667b09d699f9163 would not have happened with the other system, and the third problem (still) in that branch also would not exist.

      I consider CPP to merely be a tool. Conditional preprocessing is evil because it limits what the compiler can check, but I don't believe that writing if (PetscDefined(FOO)) { ... } is much worse than having a dynamic condition that is not tested (also bad, but less disruptive). Also, switching a configure-time conditional as above to a dynamic conditional is more local since only the predicate needs to be changed.

  2. BarryFSmith

    I hate CPP tricks, so instead of the PetscDefined() why not just have BuildSystem generate 1 or 0 for for each configure test and then the code would look like

    if (PETSC_HAVE_xxxx) { }

    no C99-style variadic macros or ugly list of macros to get the effect you want.

    For "regular" CPP macro checks you can do

    if PETSC_HAVE_xxxx

    stuff

    endif

    1. Jed Brown author

      This fails if anyone (including a user) says #ifdef PETSC_HAVE_xxx. That is really confusing. OTOH, we could get a build error when we need to reconfigure, thus finding some bugs sooner (but also making switching between branches way more painful). Anyway, the distinction between defined (possibly to 0) and truth values is confusing, error-prone, unconventional, and would break lots of existing user guards.

      1. BarryFSmith

        This is why the #ifdef syntax is sinful and should never ever be used. It should always be #if defined()

        I doubt that 1% of PETSc users have user guards (except for stupid version handling that they shouldn't do).

        Your one legitamate concern is that it is "unconventional". But you know me, I'd rather fix a 40+ year design flaw in C then be conventional. Unfortunately convention has to win out (but I will continue to remove all the #ifdef I stumble across since they are not allowed by PETSc coding standards.)

        1. Jed Brown author

          #if defined(PETSC_HAVE_xxx) is just as bad if#define PETSC_HAVE_xxx 0`.

          I have yet to interact with a major PETSc-based application that did not have guards, often for HDF5, sometimes complex, debug, etc.

          The way I see it, it's not worth bucking convention unless you have a clear and definitive win by deviating, and that it is important enough to fight the battle. It's a combined education and scripting issue. Every way in which you choose to be "special" is something that requires special cases for users and script/tool developers downstream. Users often combine packages and it is far worse to have two different paradigms than to have one, even if that one is objectively worse. There would be endless confusion and pain if we used #define PETSC_HAVE_xxx 0. I would much rather keep the status quo than have that.

          1. BarryFSmith

            You are argueing against no one. As I said above "convention has to win out"

            So you've convinced me. If this means Matt pushes far less broken code to next because he doesn't test properly then it is a worthwhile change :-)

Files changed (1)

File src/snes/examples/tutorials/ex5.c

-
 static char help[] = "Bratu nonlinear PDE in 2d.\n\
 We solve the  Bratu (SFI - solid fuel ignition) problem in a 2D rectangular\n\
 domain, using distributed arrays (DMDAs) to partition the parallel grid.\n\
 extern PetscErrorCode FormFunctionLocal(DMDALocalInfo*,PetscScalar**,PetscScalar**,AppCtx*);
 extern PetscErrorCode FormJacobianLocal(DMDALocalInfo*,PetscScalar**,Mat,Mat,AppCtx*);
 extern PetscErrorCode FormObjectiveLocal(DMDALocalInfo*,PetscScalar**,PetscReal*,AppCtx*);
-#if defined(PETSC_HAVE_MATLAB_ENGINE)
 extern PetscErrorCode FormFunctionMatlab(SNES,Vec,Vec,void*);
-#endif
 extern PetscErrorCode NonlinearGS(SNES,Vec,Vec,void*);
 
+/*
+ * Getting something that works in C and CPP for an arg that may or may not be defined is tricky.  Here, if we have
+ * "#define PETSC_HAVE_BOOGER 1" we match on the placeholder define, insert the "0," for arg1 and generate the triplet
+ * (0, 1, 0).  Then the last step cherry picks the 2nd arg (a one).  When PETSC_HAVE_BOOGER is not defined, we generate
+ * a (... 1, 0) pair, and when the last step cherry picks the 2nd arg, we get a zero.
    1. Jed Brown author

      On the last line, that will result in "ignored" expanding to PetscDefined_arg_0 1 so it is treated as false. If we want #define PETSC_HAVE_BOOGER to be interpreted like #define PETSC_HAVE_BOOGER 1, we just include both

      #define PetscDefined_arg_1 shift,
      #define PetscDefined_arg_  shift,
      

      (swapping 0, for shift, because I think the latter helps explain better).

      1. BarryFSmith

        I think it is better/less mysterious if it works regardless of what value PETSC_HAVE_BOOGER has, otherwise it does not map to

        if defined()

        can you fix it?

        1. Jed Brown author

          You want #define PETSC_HAVE_BOOGER PETSC_HAVE_BOOGER to cause the below to evaluate true? This technique can't be used for that. Perhaps PetscEnabled(BOOGER) would be better? This could check for both PETSC_HAVE_BOOGER and PETSC_USE_BOOGER.

          1. BarryFSmith

            That is an unneeded perverse special case and I don't care if the laptop reaches out and slaps the user if they try it. I just want

            define PETSC_HAVE_BOOGER anything or nothing (except PETSC_HAVE_BOOGER)

            to generate the same code regardless of what anyting is including nothing.

            PetscEnabled() is more concise. Is it "conventional"? or too unconventional?

            1. Jed Brown author

              I don't know if it's possible to get the behavior you describe. I can do it for any finite set of values. Perhaps it would make sense for our petscconf.h variables to be named differently when they are boolean versus contentful.

+ */
+#define PetscDefined_arg_1 0,
+#define PetscDefined(d)       PetscDefined_(PETSC_ ## d)
+#define PetscDefined_(d)      PetscDefined__(d)
+#define PetscDefined__(value) PetscDefined___(PetscDefined_arg_ ## value)
+#define PetscDefined___(arg1_or_junk) PetscDefined____(arg1_or_junk 1, 0)
+#define PetscDefined____(ignored, val, ...) val
+
 #undef __FUNCT__
 #define __FUNCT__ "main"
 int main(int argc,char **argv)
   PetscReal      bratu_lambda_min = 0.;
   PetscBool      flg              = PETSC_FALSE;
   DM             da;
-#if defined(PETSC_HAVE_MATLAB_ENGINE)
   Vec            r               = NULL;
   PetscBool      matlab_function = PETSC_FALSE;
-#endif
 
   /* - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
      Initialize program
     ierr = DMDASNESSetObjectiveLocal(da,(DMDASNESObjective)FormObjectiveLocal,&user);CHKERRQ(ierr);
   }
 
-#if defined(PETSC_HAVE_MATLAB_ENGINE)
-  ierr = PetscOptionsGetBool(NULL,"-matlab_function",&matlab_function,0);CHKERRQ(ierr);
-  if (matlab_function) {
-    ierr = VecDuplicate(x,&r);CHKERRQ(ierr);
-    ierr = SNESSetFunction(snes,r,FormFunctionMatlab,&user);CHKERRQ(ierr);
+  if (PetscDefined(HAVE_MATLAB_ENGINE)) {
+    ierr = PetscOptionsGetBool(NULL,"-matlab_function",&matlab_function,0);CHKERRQ(ierr);
+    if (matlab_function) {
+      ierr = VecDuplicate(x,&r);CHKERRQ(ierr);
+      ierr = SNESSetFunction(snes,r,FormFunctionMatlab,&user);CHKERRQ(ierr);
+    }
   }
-#endif
 
   /* - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
      Customize nonlinear solver; set runtime options
      Free work space.  All PETSc objects should be destroyed when they
      are no longer needed.
    - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - */
-#if defined(PETSC_HAVE_MATLAB_ENGINE)
   ierr = VecDestroy(&r);CHKERRQ(ierr);
-#endif
   ierr = VecDestroy(&x);CHKERRQ(ierr);
   ierr = SNESDestroy(&snes);CHKERRQ(ierr);
   ierr = DMDestroy(&da);CHKERRQ(ierr);
   PetscFunctionReturn(0);
 }
 
-#if defined(PETSC_HAVE_MATLAB_ENGINE)
 #undef __FUNCT__
 #define __FUNCT__ "FormFunctionMatlab"
 PetscErrorCode FormFunctionMatlab(SNES snes,Vec X,Vec F,void *ptr)
 {
+#if PetscDefined(HAVE_MATLAB_ENGINE)
    1. Jed Brown author

      There is no gain here; the gain is that everything else in the program is checked. This could also be removed if the PetscMatlab functions were defined always (to stubs that simply error). But in other cases, the body of a function like this would use types that don't exist, so the code cannot be compiled when the package is not available.

      Ideally, PETSc's public interface would be made independent of configuration, in which case new modules could be added without recompiling user code. A #ifdef in user code is tragic.

      1. BarryFSmith

        Understood. I am not rejecting your proposal, but I am noting that your proposal solves 70% of the problem and asking WHAT CAN WE DO TO SOLVE 95+% of the problem? I don't really like 70% solutions if there is any hope of a 95+% solution.

        For example, could we rig up PetscStackCallBLAS(.....) to 1) handle missing lapack so there is no ugly #if missing in the source and maybe even 2) handle real/complex so there is no #if defined(complex....)

        Are there other things that could be rigged up to handle more cases?

        One problem with allowing ANY #if def is that people generating code (one of the many PETSc developers writing code who is not you) may/will not be able to distinguish when they can avoid the #if with a trick so you may see #if appearing in the future code that need to be eraticated. In other words, the recipe people need to use to decide how to handle conditionals becomes complicated. If we eliminate all (exept possibly a very well defined class that cannot be erraticated) then it is easer for developers to do the right thing.

        In other words, this is a good idea but let's stretch as far as we can.

   AppCtx         *user = (AppCtx*)ptr;
   PetscErrorCode ierr;
   PetscInt       Mx,My;
   ierr = DMRestoreLocalVector(da,&localX);CHKERRQ(ierr);
   ierr = DMRestoreLocalVector(da,&localF);CHKERRQ(ierr);
   PetscFunctionReturn(0);
-}
+#else
+    return 0;                     /* Never called */
 #endif
+}
 
 /* ------------------------------------------------------------------- */
 #undef __FUNCT__