velocity-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From cbris...@apache.org
Subject svn commit: r1843211 - in /velocity/engine/trunk/velocity-engine-core/src: main/parser/Parser.jjt test/java/org/apache/velocity/test/BaseTestCase.java test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
Date Mon, 08 Oct 2018 22:43:48 GMT
Author: cbrisson
Date: Mon Oct  8 22:43:48 2018
New Revision: 1843211

URL: http://svn.apache.org/viewvc?rev=1843211&view=rev
Log:
[VELOCITY-889] Fix lookahead issue in macro arguments parsing

Added:
    velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
      - copied, changed from r1843186, velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity896TestCase.java
Modified:
    velocity/engine/trunk/velocity-engine-core/src/main/parser/Parser.jjt
    velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java

Modified: velocity/engine/trunk/velocity-engine-core/src/main/parser/Parser.jjt
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/main/parser/Parser.jjt?rev=1843211&r1=1843210&r2=1843211&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/main/parser/Parser.jjt (original)
+++ velocity/engine/trunk/velocity-engine-core/src/main/parser/Parser.jjt Mon Oct  8 22:43:48
2018
@@ -361,6 +361,64 @@ public class Parser
     }
 
     /**
+     * Check whether there is a right parenthesis with leading optional
+     * whitespaces. This method is used in the semantic look ahead of
+     * Directive method. This is done in code instead of as a production
+     * for simplicity and efficiency.
+     */
+    private boolean isRightParenthesis()
+    {
+        char c;
+        int no = -1;
+        try {
+            while(true)
+            {
+                /**
+                 * Read a character
+                 */
+                 if (no == -1)
+                 {
+                     switch (getToken(1).kind)
+                     {
+                        case RPAREN:
+                            return true;
+                        case WHITESPACE:
+                        case NEWLINE:
+                            no = 0;
+                            break;
+                        default:
+                            return false;
+                     }
+                 }
+                c = velcharstream.readChar();
+                no++;
+                if (c == ')')
+                {
+                    return true;
+                }
+                /**
+                 * if not a white space return
+                 */
+                else if (c != ' ' && c != '\n' && c != '\r' && c
!= '\t')
+                {
+                    return false;
+                }
+            }
+        }
+        catch(IOException e)
+        {
+        }
+        finally
+        {
+            /**
+             * Backup the stream to the initial state
+             */
+            if (no > 0) velcharstream.backup(no);
+        }
+        return false;
+    }
+
+    /**
      * We use this method in a lookahead to determine if we are in a macro
      * default value assignment.  The standard lookahead is not smart enough.
      * here we look for the equals after the reference.
@@ -1652,70 +1710,73 @@ boolean Directive() :
      /**
       * Look for the pattern [WHITESPACE] <LPAREN>
       */
-    (LOOKAHEAD( { isLeftParenthesis() } )
-    /*
-     *  if this is indeed a token, match the #foo ( arg ) pattern
-     */
-    ((<WHITESPACE> | <NEWLINE>)* <LPAREN> ( LOOKAHEAD(2) (<WHITESPACE>
| <NEWLINE>)* [<COMMA> (<WHITESPACE> | <NEWLINE>)*]
-       (
-           [LOOKAHEAD( { isMacro && isAssignment() })
-            DirectiveAssign() (<WHITESPACE> | <NEWLINE>)* <EQUALS> ( <WHITESPACE>
| <NEWLINE> )*
-           {
-               argtypes.add(ParserTreeConstants.JJTDIRECTIVEASSIGN);
-           }
-           ]
-           LOOKAHEAD( { getToken(1).kind != RPAREN } )
-           (
-            argType = DirectiveArg()
+    (
+      LOOKAHEAD( { isLeftParenthesis() } )
+      /*
+       *  if this is indeed a token, match the #foo ( arg, arg... ) pattern
+       */
+      (
+        (<WHITESPACE> | <NEWLINE>)* <LPAREN>
+        (
+          LOOKAHEAD({ !isRightParenthesis() }) (<WHITESPACE> | <NEWLINE>)* [<COMMA>
(<WHITESPACE> | <NEWLINE>)*]
+          (
+            [
+              LOOKAHEAD( { isMacro && isAssignment() })
+              DirectiveAssign() (<WHITESPACE> | <NEWLINE>)* <EQUALS> (
<WHITESPACE> | <NEWLINE> )*
+              {
+                  argtypes.add(ParserTreeConstants.JJTDIRECTIVEASSIGN);
+              }
+            ]
+            LOOKAHEAD( { !isRightParenthesis() } )
+            (
+              argType = DirectiveArg()
+              {
+                  argtypes.add(argType);
+                  if (d == null && argType == ParserTreeConstants.JJTWORD)
+                  {
+                      if (isVM)
+                      {
+                          throw new MacroParseException("Invalid argument "
+                              + (argPos+1) + " in macro call " + id.image, currentTemplate.getName(),
id);
+                      }
+                  }
+                  argPos++;
+              }
+            )
+            |
             {
-                argtypes.add(argType);
-                if (d == null && argType == ParserTreeConstants.JJTWORD)
+                if (!isMacro)
                 {
-                    if (isVM)
-                    {
-                        throw new MacroParseException("Invalid argument "
-                          + (argPos+1) + " in macro call " + id.image, currentTemplate.getName(),
id);
-                    }
+                    // We only allow line comments in macro definitions for now
+                    throw new MacroParseException("A Line comment is not allowed in " + id.image
+                        + " arguments", currentTemplate.getName(), id);
                 }
-
-                argPos++;
             }
-           )
-        |
-        {
-          if (!isMacro)
-          {
-              // We only allow line comments in macro definitions for now
-              throw new MacroParseException("A Line comment is not allowed in " + id.image
-                + " arguments", currentTemplate.getName(), id);
-          }
-        }
-
-         <SINGLE_LINE_COMMENT_START> [<SINGLE_LINE_COMMENT>]
+            <SINGLE_LINE_COMMENT_START> [<SINGLE_LINE_COMMENT>]
+          )
+        )* (<WHITESPACE> | <NEWLINE>)* <RPAREN>
       )
-    )* (<WHITESPACE> | <NEWLINE>)* <RPAREN>
-     )
-    |
-    {
-        token_source.stateStackPop();
-    }
+      |
+      {
+          token_source.stateStackPop();
+      }
     )
-     [
-       LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
-       {
-           if (directiveType == Directive.LINE)
-           {
-               jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
-               newlineAtEnd = true;
-           }
-           else
-           {
-               blockPrefix = (t == null ? u.image : t.image + u.image);
-               newlineBeforeStatement = true;
-           }
-           t = u = null;
-       }
-     ]
+    [
+      LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
+      {
+          if (directiveType == Directive.LINE)
+          {
+              jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
+              newlineAtEnd = true;
+          }
+          else
+          {
+              blockPrefix = (t == null ? u.image : t.image + u.image);
+              newlineBeforeStatement = true;
+          }
+          t = u = null;
+      }
+    ]
     {
         if (d != null)
         {
@@ -1729,32 +1790,40 @@ boolean Directive() :
     /*
      *  and the following block if the PD needs it
      */
-    (((
-        LOOKAHEAD( { getToken(1).kind != END && ( !newlineBeforeStatement || getToken(1).kind
!= WHITESPACE || getToken(2).kind != END ) }) newlineBeforeStatement = Statement(newlineBeforeStatement))*
+    (
+      (
+        (
+          LOOKAHEAD( { getToken(1).kind != END && ( !newlineBeforeStatement || getToken(1).kind
!= WHITESPACE || getToken(2).kind != END ) }) newlineBeforeStatement = Statement(newlineBeforeStatement)
+        )*
         {
             block = jjtThis;
             block.setPrefix(blockPrefix);
-        })
-        #Block)
-    [ LOOKAHEAD( 1, { newlineBeforeStatement })
-     (t = <WHITESPACE>)
+        }
+      )
+      #Block
+    )
+    [
+      LOOKAHEAD( 1, { newlineBeforeStatement })
+      (t = <WHITESPACE>)
       {
           block.setPostfix(t.image);
           t = null;
       }
     ]
-    ((end = <END>)
-     [ LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
-     {
-         jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
-         t = u = null;
-         newlineAtEnd = true;
-     }
-     ]
-     {
-         int pos = end.image.lastIndexOf('#');
-         if (pos > 0) block.setMorePostfix(end.image.substring(0, pos));
-     }
+    (
+      (end = <END>)
+      [
+        LOOKAHEAD(2) ( [ ( t = <WHITESPACE> ) ] ( u = <NEWLINE> ) )
+        {
+            jjtThis.setPostfix(t == null ? u.image : t.image + u.image);
+            t = u = null;
+            newlineAtEnd = true;
+        }
+      ]
+      {
+          int pos = end.image.lastIndexOf('#');
+          if (pos > 0) block.setMorePostfix(end.image.substring(0, pos));
+      }
     )
     {
         /*
@@ -1764,7 +1833,6 @@ boolean Directive() :
          *     as long as things were always defined before use.  This way
          *     we don't have to worry about forward references and such...
          */
-
         if (isMacro)
         {
             // Add the macro name so that we can peform escape processing
@@ -1772,16 +1840,13 @@ boolean Directive() :
             String macroName = jjtThis.jjtGetChild(0).getFirstToken().image;
             macroNames.put(macroName, macroName);
         }
-
         if (d != null)
         {
             d.checkArgs(argtypes, id, currentTemplate.getName());
         }
-
         /*
          *  VM : end
          */
-
         return newlineAtEnd;
     }
 }

Modified: velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java?rev=1843211&r1=1843210&r2=1843211&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
(original)
+++ velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/BaseTestCase.java
Mon Oct  8 22:43:48 2018
@@ -160,15 +160,6 @@ public abstract class BaseTestCase exten
         }
     }
 
-    public void testBase()
-    {
-        if (DEBUG && engine != null)
-        {
-            assertSchmoo("");
-            assertSchmoo("abc\n123");
-        }
-    }
-
     /**
      * Compare an expected string with the given loaded template
      */

Copied: velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
(from r1843186, velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity896TestCase.java)
URL: http://svn.apache.org/viewvc/velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java?p2=velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java&p1=velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity896TestCase.java&r1=1843186&r2=1843211&rev=1843211&view=diff
==============================================================================
--- velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity896TestCase.java
(original)
+++ velocity/engine/trunk/velocity-engine-core/src/test/java/org/apache/velocity/test/issues/Velocity889TestCase.java
Mon Oct  8 22:43:48 2018
@@ -24,17 +24,28 @@ import org.apache.velocity.test.BaseTest
 /**
  * This class tests VELOCITY-589.
  */
-public class Velocity896TestCase extends BaseTestCase
+public class Velocity889TestCase extends BaseTestCase
 {
-    public Velocity896TestCase(String name)
+    public Velocity889TestCase(String name)
     {
        super(name);
     }
 
-    public void testTailingHash()
+    public void testSpaceBeforeRParen()
     {
-        assertEvalEquals("#", "#");
-        assertEvalEquals("$", "$");
+        assertEvalEquals("#foo(\n)", "#foo(\n)");
+        assertEvalEquals("#foo(\n )", "#foo(\n )");
     }
 
+    public void testSpaceBeforeRParenWithArg()
+    {
+        assertEvalEquals("#foo(\n$bar\n)", "#foo(\n$bar\n)");
+        assertEvalEquals("#foo(\n $bar\n )", "#foo(\n $bar\n )");
+    }
+
+    public void testSpaceBeforeRParenWithDefaultArg()
+    {
+        assertEvalEquals("", "#macro(\nfoo\n,\n$bar\n=\n'bar')\n#end");
+        assertEvalEquals("", "#macro(\n foo\n ,\n $bar\n =\n 'bar'\n )\n #end");
+    }
 }



Mime
View raw message