[ET Trac] [Einstein Toolkit] #2050: cctk_startupxxx is a valid group

[Einstein Toolkit]

#2050: cctk_startupxxx is a valid group
----------------------+-----------------------------------------------------
  Reporter:  sbrandt  |       Owner:  sbrandt            
      Type:  defect   |      Status:  review             
  Priority:  unset    |   Milestone:                     
 Component:  Other    |     Version:  development version
Resolution:           |    Keywords:                     
----------------------+-----------------------------------------------------

Comment (by rhaas):

 Can you point to what should be reviewed? The ticket is in review state
 but you say

 > the check is not broadened by this patch

 you want to include *this* patch or the one that seems to make "at" and
 "in" behave the same way with the exception that

 1. "at" will fail if it is passed a group or bin that does not exist will
 "in" will silently not schedule a routine in that case (and for "in" this
 is the required behaviour)
 2. "at" will accept the Cactus bins with or without the leading "CCTK_"
 while "in" requires it?

 If the patch is to be applied then it is (or was) somewhat bad code in
 that it has two similar checks (for bins and groups) in the
 {{{check_schedule_database}}} and {{{parse_schedule_statement}}}
 subroutines rather than keeping similar checks in one place.

 Also, my
 > Shouldn't "AT" and "IN" be essentially identical (after applying this
 patch)?
 in comment:7 is to be answered in the negative (unless one is very loose
 in how one interprets "essentially") since AT and IN must differ in how
 they handle unknown schedule bins / groups which explains the level 0 vs.
 level 1 warning issue.

 I do not understand why {{{$level}}} is set the way it is in line 972 of
 patch. Shouldn't a non-existing bin in a "IN" statement always be non-
 fatal. Similarly I am quite confused what
 {{{$time_bin_info->{$thorn}->{$name}->{$where}}}} may be. It seems like it
 contains information about group {{{$where}}} in thorn {{{$name}}} but
 then why would there be a {{{$level}}} associated with it and why would a
 level 0 error be triggered if that {{{$level}}} is '''non-zero'''?

 It seems to me as if this patch is the result of a couple of revisions of
 the code that changed what the code wants to do and that it should be
 cleaned up to do the thing it now wants to do in a cleaner way.

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/2050#comment:12>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit