Optimize true/false conditions when coding if-statements

We already had some code which "optimized" some if-statements
with one or two conditions, where one or both were constant equal
to `true` or `false`. We skipped branches whose condition was
always `false`. However, we did not use the `IntrIgnoring` mechanism
for that. Thus, such code would still trigger warnings if it contained
undefined globals, e.g.

    gap> f:=function()  if false then undefined_global(); fi; end;;
    Syntax warning: Unbound global variable
    f:=function()  if false then undefined_global(); fi; end;;
                                                 ^

This can be viewed as a feature, or as a bug, depending on view point.
A reason to consider it a "bug" (or at least an undesirable feature)
is when using global constants to only conditionally execute code, such
as this:

    if IsHPCGAP then
        UNLOCK(lock);
    fi;

This code is optimized away in regular GAP, but still triggers a warning
during parsing.

With this commit, we change how we deal with true/false conditions in
if-statements, by making use of the `IntrIgnoring` mechanism. This avoids
the warnings about globals in the examples above. It also has the side
effect of being "better" at optimizing away branches of an if-statement
which can never be executed.

lib/system.g

@@ -168,11 +168,6 @@ fi;
 ##  For HPC-GAP, we want GAPInfo and its members to be accessible from all
 ##  threads, so make members atomic or immutable.
 ##
-if not IsHPCGAP then
-    # HACK to silence "Unbound global variable" warnings
-    AtomicList := fail;
-    AtomicRecord := fail;
-fi;
 if IsHPCGAP then
     MakeReadWriteGVar("GAPInfo");
     GAPInfo := AtomicRecord(GAPInfo);
@@ -475,13 +470,6 @@ CallAndInstallPostRestore( function()
 end );
 
 
-if not IsHPCGAP then
-    # undo HACK to silence "Unbound global variable" warnings
-    Unbind(AtomicList);
-    Unbind(AtomicRecord);
-fi;
-
-
 #############################################################################
 ##
 #V  GAPInfo.TestData

src/code.c

@@ -923,13 +923,34 @@ void CodeIfElse ( void )
 
 void CodeIfBeginBody ( void )
 {
+    // get and check the condition
+    Expr cond = PopExpr();
+
+    // if the condition is 'false', ignore the body
+    if (TNUM_EXPR(cond) == T_FALSE_EXPR) {
+        STATE(IntrIgnoring) = 1;
+    }
+    else {
+        // put the condition expression back on the stack
+        PushExpr(cond);
+    }
 }
 
-void CodeIfEndBody (
+Int CodeIfEndBody (
     UInt                nr )
 {
     /* collect the statements in a statement sequence if necessary         */
     PushStat( PopSeqStat( nr ) );
+
+    // get and check the condition
+    Expr cond = PopExpr();
+    PushExpr(cond);
+
+    // if the condition is 'true', ignore other branches of the if-statement
+    if (TNUM_EXPR(cond) == T_TRUE_EXPR) {
+        STATE(IntrIgnoring) = 1;
+    }
+    return 1;
 }
 
 void CodeIfEnd (
@@ -939,51 +960,23 @@ void CodeIfEnd (
     Expr                cond;           /* condition of a branch           */
     UInt                hase;           /* has else branch                 */
     UInt                i;              /* loop variable                   */
-    Expr                cond1 = 0;      /* first condition                 */
-    Expr                cond2 = 0;      /* second condition                */
-
-    /* peek at the last two conditions                                     */
-    cond1 = PopExpr();
-    hase = (TNUM_EXPR(cond1) == T_TRUE_EXPR);
-    if (nr == 2) {
-        cond2 = PopExpr();
-        PushExpr(cond2);
-    }
-    PushExpr(cond1);
-
-    // Some optimisation cases
-    if (nr == 1) {
-        if (TNUM_EXPR(cond1) == T_TRUE_EXPR) {
-            // Leave statement
-            PopExpr();
-            return;
-        }
-        else if (TNUM_EXPR(cond1) == T_FALSE_EXPR) {
-            // Remove entire if statement
-            PopStat();
-            PopExpr();
-            PushStat(NewStat(T_EMPTY, 0));
-            return;
-        }
+
+    // if all conditions were false, the if-statement is an empty statement
+    if (nr == 0) {
+        PushStat(NewStat(T_EMPTY, 0));
+        return;
     }
 
-    if (nr == 2 && hase) {
-        if (TNUM_EXPR(cond2) == T_TRUE_EXPR) {
-            // Leave 'true' case
-            PopStat();
-            PopExpr();
-            PopExpr();
-            return;
-        }
-        else if (TNUM_EXPR(cond2) == T_FALSE_EXPR) {
-            // Leave 'false' case
-            Stat body = PopStat();
-            PopExpr();
-            PopStat();
-            PopExpr();
-            PushStat(body);
-            return;
-        }
+    // peek at the last condition
+    cond = PopExpr();
+    hase = (TNUM_EXPR(cond) == T_TRUE_EXPR);
+    PushExpr(cond);
+
+    // optimize 'if true then BODY; fi;' to just 'BODY;'
+    if (nr == 1 && hase) {
+        // drop the condition expression, leave the body statement
+        PopExpr();
+        return;
     }
 
     /* allocate the if-statement                                           */

src/code.h

@@ -692,7 +692,7 @@ extern  void            CodeIfElse ( void );
 
 extern  void            CodeIfBeginBody ( void );
 
-extern  void            CodeIfEndBody (
+extern  Int             CodeIfEndBody (
             UInt                nr );
 
 extern  void            CodeIfEnd (

src/intrprtr.c

@@ -547,6 +547,13 @@ void            IntrIfBegin ( void )
 {
     /* ignore or code                                                      */
     if ( STATE(IntrReturning) > 0 ) { return; }
+
+    // if IntrIgnoring is positive, increment it, as IntrIgnoring == 1 has a
+    // special meaning when parsing if-statements -- it is used to skip
+    // interpreting or coding branches of the if-statement which never will be
+    // executed, either because a previous branch is always executed (i.e., it
+    // has a 'true' condition), or else because the current branch has a
+    // 'false' condition
     if ( STATE(IntrIgnoring)  > 0 ) { STATE(IntrIgnoring)++; return; }
     if ( STATE(IntrCoding)    > 0 ) { CodeIfBegin(); return; }
 
@@ -597,22 +604,15 @@ void            IntrIfBeginBody ( void )
     }
 }
 
-void            IntrIfEndBody (
+Int            IntrIfEndBody (
     UInt                nr )
 {
     UInt                i;              /* loop variable                   */
 
     /* ignore or code                                                      */
-    if ( STATE(IntrReturning) > 0 ) { return; }
-    if ( STATE(IntrIgnoring)  > 1 ) { STATE(IntrIgnoring)--; return; }
-    if ( STATE(IntrCoding)    > 0 ) { CodeIfEndBody( nr ); return; }
-
-
-    /* if the condition was 'false', the body was ignored                  */
-    if ( STATE(IntrIgnoring) == 1 ) {
-        STATE(IntrIgnoring) = 0;
-        return;
-    }
+    if ( STATE(IntrReturning) > 0 ) { return 0; }
+    if ( STATE(IntrIgnoring)  > 0 ) { STATE(IntrIgnoring)--; return 0; }
+    if ( STATE(IntrCoding)    > 0 ) { return CodeIfEndBody( nr ); }
 
     /* otherwise drop the values for the statements executed in the body   */
     for ( i = nr; 1 <= i; i-- ) {
@@ -621,6 +621,8 @@ void            IntrIfEndBody (
 
     /* one branch of the if-statement was executed, ignore the others      */
     STATE(IntrIgnoring) = 1;
+
+    return 1;
 }
 
 void            IntrIfEnd (
@@ -629,19 +631,15 @@ void            IntrIfEnd (
     /* ignore or code                                                      */
     if ( STATE(IntrReturning) > 0 ) { return; }
     if ( STATE(IntrIgnoring)  > 1 ) { STATE(IntrIgnoring)--; return; }
-    if ( STATE(IntrCoding)    > 0 ) { CodeIfEnd( nr ); return; }
-
 
-    /* if one branch was executed (ignoring the others)                    */
+    // if one branch was executed (ignoring the others), reset IntrIgnoring
     if ( STATE(IntrIgnoring) == 1 ) {
         STATE(IntrIgnoring) = 0;
-        PushVoidObj();
     }
 
-    /* if no branch was executed                                           */
-    else {
-        PushVoidObj();
-    }
+    if ( STATE(IntrCoding)    > 0 ) { CodeIfEnd( nr ); return; }
+
+    PushVoidObj();
 }
 
 

src/intrprtr.h

@@ -204,7 +204,7 @@ extern  void            IntrIfElse ( void );
 
 extern  void            IntrIfBeginBody ( void );
 
-extern  void            IntrIfEndBody (
+extern  Int             IntrIfEndBody (
             UInt                nr );
 
 extern  void            IntrIfEnd (

src/read.c

@@ -2124,8 +2124,7 @@ void ReadIf (
     Match( S_THEN, "then", STATBEGIN|S_ELIF|S_ELSE|S_FI|follow );
     TRY_READ { IntrIfBeginBody(); }
     nrs = ReadStats( S_ELIF|S_ELSE|S_FI|follow );
-    TRY_READ { IntrIfEndBody( nrs ); }
-    nrb++;
+    TRY_READ { nrb += IntrIfEndBody( nrs ); }
 
     /* { 'elif' <Expr>  'then' <Statments> }                               */
     while ( STATE(Symbol) == S_ELIF ) {
@@ -2135,8 +2134,7 @@ void ReadIf (
         Match( S_THEN, "then", STATBEGIN|S_ELIF|S_ELSE|S_FI|follow );
         TRY_READ { IntrIfBeginBody(); }
         nrs = ReadStats( S_ELIF|S_ELSE|S_FI|follow );
-        TRY_READ { IntrIfEndBody( nrs ); }
-        nrb++;
+        TRY_READ { nrb += IntrIfEndBody( nrs ); }
     }
 
     /* [ 'else' <Statments> ]                                              */
@@ -2145,8 +2143,7 @@ void ReadIf (
         Match( S_ELSE, "else", follow );
         TRY_READ { IntrIfBeginBody(); }
         nrs = ReadStats( S_FI|follow );
-        TRY_READ { IntrIfEndBody( nrs ); }
-        nrb++;
+        TRY_READ { nrb += IntrIfEndBody( nrs ); }
     }
 
     /* 'fi'                                                                */