Respond to @smharper comments on #463

openmc/region.py

@@ -67,7 +67,8 @@ class Region(object):
             else:
                 # Check for invalid characters
                 if expression[i] not in '-0123456789':
-                    raise SyntaxError('Invalid character in expression')
+                    raise SyntaxError("Invalid character '{}' in expression"
+                                      .format(expression[i]))
 
                 # If we haven't yet reached the start of a word, start one
                 if i_start < 0:

src/geometry.F90

@@ -83,7 +83,6 @@ contains
 
     integer :: i
     integer :: token
-    logical :: b1, b2
     integer :: i_stack
     logical :: actual_sense    ! sense of particle wrt surface
     logical :: stack(size(c%rpn))
@@ -91,7 +90,20 @@ contains
     i_stack = 0
     do i = 1, size(c%rpn)
       token = c%rpn(i)
-      if (token < OP_UNION) then
+
+      ! If the token is a binary operator (intersection/union), apply it to
+      ! the last two items on the stack. If the token is a unary operator
+      ! (complement), apply it to the last item on the stack.
+      select case (token)
+      case (OP_UNION)
+        stack(i_stack - 1) = stack(i_stack - 1) .or. stack(i_stack)
+        i_stack = i_stack - 1
+      case (OP_INTERSECTION)
+        stack(i_stack - 1) = stack(i_stack - 1) .and. stack(i_stack)
+        i_stack = i_stack - 1
+      case (OP_COMPLEMENT)
+        stack(i_stack) = .not. stack(i_stack)
+      case default
         ! If the token is not an operator, evaluate the sense of particle with
         ! respect to the surface and see if the token matches the sense. If the
         ! particle's surface attribute is set and matches the token, that
@@ -106,24 +118,7 @@ contains
                p%coord(p%n_coord)%xyz, p%coord(p%n_coord)%uvw)
           stack(i_stack) = (actual_sense .eqv. (token > 0))
         end if
-      else
-        ! If the token is a binary operator (intersection/union), apply it to
-        ! the last two items on the stack. If the token is a unary operator
-        ! (complement), apply it to the last item on the stack.
-        b1 = stack(i_stack)
-        select case (token)
-        case (OP_UNION)
-          b2 = stack(i_stack - 1)
-          stack(i_stack - 1) = b1 .or. b2
-          i_stack = i_stack - 1
-        case (OP_INTERSECTION)
-          b2 = stack(i_stack - 1)
-          stack(i_stack - 1) = b1 .and. b2
-          i_stack = i_stack - 1
-        case (OP_COMPLEMENT)
-          stack(i_stack) = .not. b1
-        end select
-      end if
+      end select
     end do
 
     if (i_stack == 1) then
@@ -630,11 +625,11 @@ contains
       ! FIND MINIMUM DISTANCE TO SURFACE IN THIS CELL
 
       SURFACE_LOOP: do i = 1, size(cl%region)
-        ! check for operators
         index_surf = cl%region(i)
         coincident = (index_surf == p % surface)
 
-        ! check for operators
+        ! ignore this token if it corresponds to an operator rather than a
+        ! region.
         index_surf = abs(index_surf)
         if (index_surf >= OP_UNION) cycle
 
@@ -642,7 +637,7 @@ contains
         surf => surfaces(index_surf)%obj
         d = surf%distance(p%coord(j)%xyz, p%coord(j)%uvw, coincident)
 
-        ! Check is calculated distance is new minimum
+        ! Check if calculated distance is new minimum
         if (d < d_surf) then
           if (abs(d - d_surf)/d_surf >= FP_PRECISION) then
             d_surf = d
@@ -899,8 +894,11 @@ contains
       do j = 1, size(c%region)
         i_surface = c % region(j)
         positive = (i_surface > 0)
+
+        ! Skip any tokens that correspond to operators rather than regions
         i_surface = abs(i_surface)
         if (i_surface >= OP_UNION) cycle
+
         if (positive) then
           count_positive(i_surface) = count_positive(i_surface) + 1
         else
@@ -930,6 +928,8 @@ contains
       do j = 1, size(c%region)
         i_surface = c % region(j)
         positive = (i_surface > 0)
+
+        ! Skip any tokens that correspond to operators rather than regions
         i_surface = abs(i_surface)
         if (i_surface >= OP_UNION) cycle
 

src/initialize.F90

@@ -573,6 +573,9 @@ contains
       c => cells(i)
       do j = 1, size(c%region)
         id = c%region(j)
+        ! Make sure that only regions are checked. Since OP_UNION is the
+        ! operator with the lowest integer value, anything below it must denote
+        ! a half-space
         if (id < OP_UNION) then
           if (surface_dict%has_key(abs(id))) then
             i_array = surface_dict%get_key(abs(id))
@@ -584,8 +587,10 @@ contains
         end if
       end do
 
+      ! Also adjust the indices in the reverse Polish notation
       do j = 1, size(c%rpn)
         id = c%rpn(j)
+        ! Again, make sure that only regions are checked
         if (id < OP_UNION) then
           i_array = surface_dict%get_key(abs(id))
           c%rpn(j) = sign(i_array, id)

src/summary.F90

@@ -179,21 +179,19 @@ contains
       region_spec = ""
       do j = 1, size(c%region)
         k = c%region(j)
-        if (k < OP_UNION) then
+        select case(k)
+        case (OP_LEFT_PAREN)
+          region_spec = trim(region_spec) // " ("
+        case (OP_RIGHT_PAREN)
+          region_spec = trim(region_spec) // " )"
+        case (OP_COMPLEMENT)
+          region_spec = trim(region_spec) // " ~"
+        case (OP_UNION)
+          region_spec = trim(region_spec) // " ^"
+        case default
           region_spec = trim(region_spec) // " " // to_str(&
                sign(surfaces(abs(k))%obj%id, k))
-        else
-          select case(k)
-          case (OP_LEFT_PAREN)
-            region_spec = trim(region_spec) // " ("
-          case (OP_RIGHT_PAREN)
-            region_spec = trim(region_spec) // " )"
-          case (OP_COMPLEMENT)
-            region_spec = trim(region_spec) // " ~"
-          case (OP_UNION)
-            region_spec = trim(region_spec) // " ^"
-          end select
-        end if
+        end select
       end do
       call write_dataset(cell_group, "region", adjustl(region_spec))