Bug 45041 - improved FormulaParser parse error messages

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@659452 13f79535-47bb-0310-9956-ffa450edef68

src/documentation/content/xdocs/changes.xml

@@ -37,6 +37,7 @@
 
 		<!-- Don't forget to update status.xml too! -->
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="add">45041 - improved FormulaParser parse error messages</action>
            <action dev="POI-DEVELOPERS" type="add">45046 - allowed EXTERNALBOOK(0x01AE) to be optional in the LinkTable</action>
            <action dev="POI-DEVELOPERS" type="add">45066 - fixed sheet encoding size mismatch problems</action>
            <action dev="POI-DEVELOPERS" type="add">45003 - Support embeded HDGF visio documents</action>

src/documentation/content/xdocs/status.xml

@@ -34,6 +34,7 @@
 	<!-- Don't forget to update changes.xml too! -->
     <changes>
         <release version="3.1-final" date="2008-06-??">
+           <action dev="POI-DEVELOPERS" type="add">45041 - improved FormulaParser parse error messages</action>
            <action dev="POI-DEVELOPERS" type="add">45046 - allowed EXTERNALBOOK(0x01AE) to be optional in the LinkTable</action>
            <action dev="POI-DEVELOPERS" type="add">45066 - fixed sheet encoding size mismatch problems</action>
            <action dev="POI-DEVELOPERS" type="add">45003 - Support embeded HDGF visio documents</action>

src/java/org/apache/poi/hssf/model/FormulaParser.java

@@ -55,7 +55,7 @@ public final class FormulaParser {
      */
     static final class FormulaParseException extends RuntimeException {
         // This class was given package scope until it would become clear that it is useful to
-        // general client code. 
+        // general client code.
         public FormulaParseException(String msg) {
             super(msg);
         }
@@ -127,14 +127,14 @@ public final class FormulaParser {
             // Just return if so and reset 'look' to something to keep
             // SkipWhitespace from spinning
             look = (char)0;
-        }    
+        }
         pointer++;
         //System.out.println("Got char: "+ look);
     }
 
     /** Report What Was Expected */
     private RuntimeException expected(String s) {
-        String msg = "Parse error near char " + (pointer-1) + "'" + look + "'" 
+        String msg = "Parse error near char " + (pointer-1) + " '" + look + "'"
             + " in specified formula '" + formulaString + "'. Expected "
             + s;
         return new FormulaParseException(msg);
@@ -178,7 +178,7 @@ public final class FormulaParser {
     /**
      *  Consumes the next input character if it is equal to the one specified otherwise throws an
      *  unchecked exception. This method does <b>not</b> consume whitespace (before or after the
-     *  matched character). 
+     *  matched character).
      */
     private void Match(char x) {
         if (look != x) {
@@ -281,18 +281,18 @@ public final class FormulaParser {
         // This can be either a cell ref or a named range
         // Try to spot which it is
         boolean cellRef = CELL_REFERENCE_PATTERN.matcher(name).matches();
- 
+
         if (cellRef) {
             return new ReferencePtg(name);
         }
 
         for(int i = 0; i < book.getNumberOfNames(); i++) {
             // named range name matching is case insensitive
-        	if(book.getNameAt(i).getNameName().equalsIgnoreCase(name)) {
+            if(book.getNameAt(i).getNameName().equalsIgnoreCase(name)) {
                 return new NamePtg(name, book);
             }
         }
-        throw new FormulaParseException("Found reference to named range \"" 
+        throw new FormulaParseException("Found reference to named range \""
                     + name + "\", but that named range wasn't defined!");
     }
 
@@ -307,19 +307,19 @@ public final class FormulaParser {
     /**
      * Note - Excel function names are 'case aware but not case sensitive'.  This method may end
      * up creating a defined name record in the workbook if the specified name is not an internal
-     * Excel function, and has not been encountered before. 
-     * 
-     * @param name case preserved function name (as it was entered/appeared in the formula). 
+     * Excel function, and has not been encountered before.
+     *
+     * @param name case preserved function name (as it was entered/appeared in the formula).
      */
     private Ptg function(String name) {
         int numArgs =0 ;
-        // Note regarding parameter - 
+        // Note regarding parameter -
         if(!AbstractFunctionPtg.isInternalFunctionName(name)) {
             // external functions get a Name token which points to a defined name record
             NamePtg nameToken = new NamePtg(name, this.book);
-            
+
             // in the token tree, the name is more or less the first argument
-            numArgs++;  
+            numArgs++;
             tokens.add(nameToken);
         }
         //average 2 args per function
@@ -477,26 +477,25 @@ public final class FormulaParser {
     private static boolean isArgumentDelimiter(char ch) {
         return ch ==  ',' || ch == ')';
     }
-    
+
     /** get arguments to a function */
     private int Arguments(List argumentPointers) {
         SkipWhite();
         if(look == ')') {
             return 0;
         }
-        
+
         boolean missedPrevArg = true;
-        
         int numArgs = 0;
-        while(true) {
+        while (true) {
             SkipWhite();
-            if(isArgumentDelimiter(look)) {
-                if(missedPrevArg) {
+            if (isArgumentDelimiter(look)) {
+                if (missedPrevArg) {
                     tokens.add(new MissingArgPtg());
                     addArgumentPointer(argumentPointers);
                     numArgs++;
                 }
-                if(look == ')') {
+                if (look == ')') {
                     break;
                 }
                 Match(',');
@@ -507,6 +506,10 @@ public final class FormulaParser {
             addArgumentPointer(argumentPointers);
             numArgs++;
             missedPrevArg = false;
+            SkipWhite();
+            if (!isArgumentDelimiter(look)) {
+                throw expected("',' or ')'");
+            }
         }
         return numArgs;
     }
@@ -524,7 +527,7 @@ public final class FormulaParser {
             tokens.add(new PowerPtg());
         }
     }
-    
+
     private void percentFactor() {
         tokens.add(parseSimpleFactor());
         while(true) {
@@ -536,8 +539,8 @@ public final class FormulaParser {
             tokens.add(new PercentPtg());
         }
     }
-    
-    
+
+
     /**
      * factors (without ^ or % )
      */
@@ -710,7 +713,7 @@ public final class FormulaParser {
     private StringPtg parseStringLiteral()
     {
         Match('"');
-        
+
         StringBuffer token = new StringBuffer();
         while (true) {
             if (look == '"') {
@@ -745,7 +748,7 @@ public final class FormulaParser {
             return; // finished with Term
         }
     }
-    
+
     private void comparisonExpression() {
         concatExpression();
         while (true) {
@@ -787,7 +790,7 @@ public final class FormulaParser {
         }
         return new LessThanPtg();
     }
-    
+
 
     private void concatExpression() {
         additiveExpression();
@@ -801,7 +804,7 @@ public final class FormulaParser {
             tokens.add(new ConcatPtg());
         }
     }
-    
+
 
     /** Parse and Translate an Expression */
     private void additiveExpression() {
@@ -847,8 +850,8 @@ end;
         comparisonExpression();
 
         if(pointer <= formulaLength) {
-            String msg = "Unused input [" + formulaString.substring(pointer-1) 
-                + "] after attempting to parse the formula [" + formulaString + "]"; 
+            String msg = "Unused input [" + formulaString.substring(pointer-1)
+                + "] after attempting to parse the formula [" + formulaString + "]";
             throw new FormulaParseException(msg);
         }
     }
@@ -1011,7 +1014,7 @@ end;
                     continue;
                     // but if it ever did, care must be taken:
                     // tAttrSpace comes *before* the operand it applies to, which may be consistent
-                    // with how the formula text appears but is against the RPN ordering assumed here 
+                    // with how the formula text appears but is against the RPN ordering assumed here
                 }
                 if (attrPtg.isSemiVolatile()) {
                     // similar to tAttrSpace - RPN is violated
@@ -1038,7 +1041,7 @@ end;
             stack.push(o.toFormulaString(operands));
         }
         if(stack.isEmpty()) {
-            // inspection of the code above reveals that every stack.pop() is followed by a 
+            // inspection of the code above reveals that every stack.pop() is followed by a
             // stack.push(). So this is either an internal error or impossible.
             throw new IllegalStateException("Stack underflow");
         }