CPP options set/unset in mutltiple header files

[jm-c]

This is mostly for AD set-up: I noticed that some (CPP) options could be define/undef in in different option-hear files, which is risky since then it does matter in with order they are included.
For instance, in AD exp. "1D_ocean_ice_column", I found:
ECCO_OPTIONS.h:#define ALLOW_GENCOST_CONTRIBUTION
COST_OPTIONS.h:#undef ALLOW_GENCOST_CONTRIBUTION
and:
AUTODIFF_OPTIONS.h:#undef ALLOW_PACKUNPACK_METHOD2
CTRL_OPTIONS.h:#define ALLOW_PACKUNPACK_METHOD2
To be more explicit, in few pkg/ctrl src file, AUTODIFF_OPTIONS.h is included after CTRL_OPTIONS.h resulting in ALLOW_PACKUNPACK_METHOD2 beeing #undef (despite beeing #define in CTRL_OPTIONS) in these src file.

I think it would be helpful to make it clear where a CPP option is set/unset, and to check that multiple setting of the same
option does not happen.

[jm-c]

I cooked a little script that helps finding options that are set/unset in several places. I only tried from all AD verification experiment "build" dir (but this covers most of the code). I think it would be useful to sort out the few cases where this occurs.

The other related task would be make sure most options that are found somewhere in the code are listed (#undef) in one specific (default) CPP header file (so that a user would know where to #define it). I have an other script that I can run to help to find these options.

[mjlosch]

Sounds like a good plan. In many cases it's obvious (e.g. ALLOW_PACKUNPACK_METHOD2 related to pkg/ctrl and should not be in AUTODIFF_OPTIONS.h). It would be good to have a list of places where this happens. Does it make sense to post it here, since you (@jm-c) already have a script that finds double definitions?
I also agree that having all "definable" cpp-flags listed in the appropriate $PKG_OPTIONS.h file, as "undef" if that's the default, is a very good idea.

Part of this is done (inefficiently) in some PRs, but it would be good to have a dedicated PR for this.

[mjlosch]

I can start a PR with this, but there are a few things that are unclear to me:

1. pkg/cost is an alternative to pkg/ecco, but still these packages seem to be entangled (you need cost to use ecco?)
2. many ecco-related cost function flags are set in COST_OPTIONS.h, I think these should be moved to ECCO_OPTIONS.h
3. many of these flags are no longer in use (?), only in cost_check.F (which is a file that contains only obsolete ctrl flags and no real code. What should we do with this? Delete?
4. Some flags are documented but never used (e.g. I found ALLOW_GENCOST_FREEFORM in COST_OPTIONS.h but not anywhere else in the code), we could move these flags to the right place (pkg/ecco/ECCO_OPTIONS.h), or remove them altogether (which I prefer).

Should I just start a (draft) PR and we discuss this as we go along?

[jm-c]

@mjlosch At some point I did check for CPP options that are defined or undef in several header files
but this was before PR #631 got merged. I am going to re-do this and will report here what I found.

[jm-c]

I went back to check for Options defined/undef in multiple header files:

1. From default CPP header files: ALLOW_GENCOST_CONTRIBUTION is undef in COST_OPTIONS.h and defined in ECCO_OPTIONS.h. Since this option only appears in pkg/ecco src files, it makes sense to remove it from COST_OPTIONS.h.
   And then these 3 options: CARBONCHEM_SOLVESAPHE, CARBONCHEM_TOTALPHSCALE and USE_QSW are used in both pkg/bling and pkg/dic (and set in respective *_OPTIONS.h header file). In this case, we could just make sure these 2 pkgs are never compiled together (in pkg_depend, e.g., excluding dic if compiling bling).
2. From AD experiment build dir: as mentioned earlier, ALLOW_PACKUNPACK_METHOD2 is def/undef in few experiments (1D_ocean_ice_column, global_oce_biogeo_bling, lab_sea, tutorial_global_oce_optim) in local CTRL_OPTIONS.h and AUTODIFF_OPTIONS.h.
   ALLOW_GENCOST_CONTRIBUTION is also def/undef in all exp. that use pkg/ecco
   (not surprising). And lab_sea has still ALLOW_{THETA,SALT,SST,SSS}_COST_CONTRIBUTION 4 options def in (local) ECCO_OPTIONS.h and undef in (default) COST_OPTIONS.h.
3. The other thing is when an Option setting depends on an other Option (most of
   the time, all in same header file): Since users might need to customize CPP header files, it would be useful to add a comment about this dependence between the 2 options. Sometimes, Option-1 needs to be set when Option-2 is defined, e.g., in exp. obcs_ctrl CTRL_OPTIONS.h, ALLOW_OBCS_CONTROL is defined (as a shortcut) when one or more N,S,E,W specific OB control is defined (not to be changed). Or in GCHEM_OPTIONS.h, GCHEM_ADD2TR_TENDENCY needs to be defined to use CFC.
   In other places, it's just that Option-1 is only affecting code when Option-2 is set, e.g., in SEAICE_OPTIONS.h, ALLOW_SITRACER_ADVCAP affecting ALLOW_SITRACER code, and a user might turn ADVCAP on or off when ALLOW_SITRACER is defined.
4. And when an Option is potentially set multiple times within the same header file, some depending on an other option, it could be mis-leading that changing the setting in one place does not do anything. E.g., in BLING_OPTIONS.h, turning on USE_BLING_DVM on line 19 has no effect (still undef) since BLING_ADJOINT_SAFE is defined (line 68) and turn off USE_BLING_DVM again (line 72). Might be sometimes tricky to improve but some re-ordering of setting could help.

[jm-c]

Also, I went through the list of all CPP options in the code (720 something), found 2 typo, and plan to open a new PR to clean and fix CPP options. My plan would be to address most of the points above, fix typo and set as "#undef" some of the options that are found in the code but not set anywhere (currently about 180) since:

1. it's useful when/if a user want to use an option to know where it's supposed to be turned on.
2. it does help to find inconsistent setting (or typo) by reducing the number of options that are not set anywhere and need to be checked.

@mjlosch do you agree or should we proceed differently ?

[mjlosch]

@jm-c thanks for taking the initiative here.

[jm-c]

Multiple CPP option setting have been fixed (at least in reference CPP header files, not yet in all verfication exp. code dir).
I am going to open a new PR to remove orphan code that has been left behind within "#ifdef" of retired/obsolete CPP option.

[jm-c]

All points raised in this issue have been addressed in PR #689 and #711.