Opened 3 months ago

Last modified 4 weeks ago

#2192 assigned defect

schedule.ccl SYNC allows for [timelevels] suffixes

Reported by: zachetie@… Owned by: Steven R. Brandt
Priority: unset Milestone:
Component: Other Version: development version
Keywords: piraha Cc:


This schedule.ccl line (from an older version of GiRaFFE) should not have been permitted by the scheduler:

SYNC: GiRaFFE::grmhd_primitives_Bi, GiRaFFE::grmhd_primitives_allbutBi, GiRaFFE::em_Ax[3],GiRaFFE::em_Ay[3],GiRaFFE::em_Az[3],GiRaFFE::em_psi6phi[3],GiRaFFE::grmhd_conservatives[3],GiRaFFE::BSSN_quantities,ADMBase::metric,ADMBase::lapse,ADMBase::shift,ADMBase::curv

Attachments (1)

vname.patch (2.4 KB) - added by Steven R. Brandt 5 weeks ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 months ago by Roland Haas

Keywords: piraha added
Owner: set to Steven R. Brandt
Status: newassigned

This looks like a failure in the piraha parser to me.

comment:2 Changed 5 weeks ago by Steven R. Brandt

Just to clarify, the correct thing is to disallow the square brackets. Correct?

comment:3 Changed 5 weeks ago by Roland Haas

I think so, yes. The syntax of schedule.ccl files is defined in the UserGuide ( where it says

 schedule [GROUP] <function name|group name> AT|IN <time> \
     [AS <alias>] \
     [WHILE <variable>] [IF <variable>] \
     [BEFORE|AFTER <function name>|(<function name> <function name> ...)] \
  [LANG: <language>]
  [OPTIONS:       <option>,<option>...]
  [TAGS:          <keyword=value>,<keyword=value>...]
  [STORAGE:       <group>[timelevels],<group>[timelevels]...]
  [READS:         <group>,<group>...]
  [WRITES:        <group>,<group>...]
  [TRIGGER:       <group>,<group>...]
  [SYNCHRONISE:   <group>,<group>...]
  [OPTIONS:       <option>,<option>...]
} "Description of function" 


    A group of grid variables. Variable groups inherited from other
thorns may be used, but they must then be fully qualified with the
implementation name. 

and a group name is defined in interface.ccl's doc ( where is says

<data_type> <group_name>[[<number>]] [TYPE=<group_type>] [DIM=<dim>]
[SIZE=<size in each direction>] [DISTRIB=<distribution_type>]
[TAGS=<string>]  ["<group_description>"]
 [ <variable_name>[,]<variable_name>
   <variable_name> ]
} ["<group_description>"] ]

* group_name must be an alphanumeric name (which may also contain
underscores) which is unique across group and variable names within
the scope of the thorn.

Meaning "forbid everything that is not either a alphanumeric string or such a string followed by :: and an alphanumeric string".

comment:4 Changed 5 weeks ago by Steven R. Brandt

The following patch to the flesh addresses this:

diff --git a/src/piraha/pegs/schedule.peg b/src/piraha/pegs/schedule.peg
index e24175c..990289a 100644
--- a/src/piraha/pegs/schedule.peg
+++ b/src/piraha/pegs/schedule.peg
@@ -6,6 +6,7 @@ name = (?i:[a-zA-Z_][a-zA-Z0-9_\-]*\b)
 expr = {name}|{num}
 # TODO: Should this be a * or a ?
 vname = {name}( :: {name})*( \[ {expr} \]|)
+uname = {name}( :: {name})*
 quote = "(\\{any}|[^"])*"
 ccomment = /\*((?!\*/){-any})*\*/
 num = [+\-]?[0-9]+(\.[0-9]+)?
@@ -40,7 +41,7 @@ group = (?i:group)
 nogroup =
 prepositions = ({preposition} )*
 preposition = {par} {pararg}
-sync = (?i:sync) : {vname}( , {vname}|[ \t]{vname})*
+sync = (?i:sync) : {uname}( , {uname}|[ \t]{uname})*
 options = (?i:options?) : {vname}( , {vname}|[ \t]{vname})*
 storage = (?i:storage) : {vname}( , {vname}|[ \t]{vname})*
 triggers = (?i:triggers?) : {vname}( , {vname}|[ \t]{vname})*

comment:5 Changed 5 weeks ago by Roland Haas

Wouldn't this pattern uname also accept: foo::bar::baz which should not accept? I would suggest to mabye naming the pattern gname for "group" name assuming that vname stood for "variable name".

comment:6 Changed 5 weeks ago by Steven R. Brandt

It would. I can replace the * with ?.

I thought there was a case where we wanted a::b::c. I've just checked, though, that if I change both vname and uname to use ? instead of * the ET compiles.

comment:7 Changed 5 weeks ago by Roland Haas

'?' is the correct modifier I agree. I had not noticed the comment already in the code about whether '?' or '*' should be used.

I would say that when in doubt then I would start by consulting the appendices of the UserGuide which define the grammar of the ccl file and first try adhering that them. If that fails to compile the ET then either the ET has a bug (and needs fixing) or the grammar in the appendices (and in the peg files) needs to be updated b/c there was a documentation bug.

Looking at your proposed change, I think also eg for the storage keyword one should use uname since storage is also controlled group-wide and not variable per variable.

Changed 5 weeks ago by Steven R. Brandt

Attachment: vname.patch added

comment:8 Changed 5 weeks ago by Roland Haas

Thinking about this a bit more, while indeed STORAGE takes a group name, it does accept expressions in square brackets afterwards, namely the number of timelevels for which one allocates storage. How does piraha extract those right now? Would the current patch compile and pass the test suites?

I also notice that in the patch in line 5 the "name" pattern is

name = (?i:[a-zA-Z_][a-zA-Z0-9_\-]*\b)

ie it accepts "-" as a valid part of a name, which is not true for variable names (since they must be valid C identifiers) and also not really true for group names (which are alphanumeric plus the underscore).

comment:9 Changed 4 weeks ago by Steven R. Brandt

No, the current patch doesn't pass. I'm working on it.

Modify Ticket

Change Properties
Set your email in Preferences
as assigned The owner will remain Steven R. Brandt.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Steven R. Brandt to the specified user.
The owner will be changed from Steven R. Brandt to anonymous.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.