https://issues.apache.org/bugzilla/show_bug.cgi?id=48292
Josh Micich <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #24645|0 |1 is obsolete| | Attachment #24658|0 |1 is obsolete| | --- Comment #6 from Josh Micich <[email protected]> 2009-12-09 00:16:06 UTC --- Created an attachment (id=24682) --> (https://issues.apache.org/bugzilla/attachment.cgi?id=24682) another draft of the patch Thanks for all your work so far but I'm afraid it's going to take a few more iterations before we are able to commit this patch. Please take a look at my latest reworked version. I have made several changes which are described below. There are still outstanding issues that need to be addressed. A very small amount of work has been submitted (svn r888577, svn r888582, svn r888665). These changes don't add any functionality; they just resolve existing issues and make the pending patch simpler. Firstly, I've added Apache License notices to each new file. It's important that you agree to this, because we cannot take code contributions without it. The tests you provided were in a package "com.exigen...". These have been moved to the appropriate POI packages. The tests needed to be converted to junit 3.8 because that's what POI uses (changing junit versions is probably something we'd decide to do project wide). The test classes were re-named to follow POI conventions, though the names could still be more descriptive. POI expects tests to be silent (not write to std-out or any log) when successful. Test code should not throw exceptions to be caught by other test code during successful tests. The only exceptions that should be thrown are ones deliberately caused in the production code being tested. One test method for every example spreadsheet cell is probably too many. Some simplification was done, but consider using techniques like those in TestFormulasFromSpreadsheet and TestIndexFunctionFromSpreadsheet etc. General coding/formatting guidelines: - please use unix line endings (LF instead of CRLF) - don't mix indent style (tabs or spaces) in the same file - always use {} for the bodies of if/else for/while statements - don't assign to parameters - turn on IDE warnings for unnecessary code I made significant simplifications to ArrayEval (while keeping all your junit tests working). I got rid of ArrayEval.illegalForAggregation because there is a much simpler way to get #N/A out of formulas with array size errors: The #N/A value originates in ArrayFormulaEvaluatorHelper.getArrayValue() (new method), and just flows up the evaluation stack through standard techniques. The method ArrayEval.arrayAsArea() is also not needed (due to introduction of TwoDEval). TwoDEval simplifies a lot of the code you needed to touch in existing function implementations. There are also some problems in your interpretation of how array formulas work. I am pretty sure that at least one of the test cases is asserting the wrong result (testOffsetArray() - ConstantArray.xls C149). I have added a new test class TestArrayEvaluationExtra which shows what the correct behaviour should be. The new failing test cases are currently disabled. While investigating, I have found out that array eval elements cannot be cell references or areas. So in this example, OFFSET cannot return an array of cell references. I've disabled ArrayEval from doing this, and made other fixes to keep all test cases working. The solution will probably involve getting the RVA types of the tokens correct during the formula parsing phase. Read the OOO documentation for an introduction to this. Unfortunately I think that information is incomplete, but it is the best resource I know of. We will probably have to discover all the missing details for ourselves. I also found that using the "Evaluate Formula" button (available in Excel 2007) helped me understand how some of the example formulas should be evaluated. The short-circuit-if evaluation stuff is probably not needed. The first parameter to IF has type 'V' and I have a feeling that any parameter of type 'V' should always have a scalar ValueEval passed at runtime. If you pass an array for a parameter of type 'V' then the whole function gets invoked once for each item in the array. You have already written some code (prepareArgsForLoop etc) that does this. It's just a question of having it be invoked at the right time. There are a few other problems in the code: Don't instantiate ArrayEval until you have its contents calculated. (i.e. don't create half-initialised objects). FunctionWithArraySupport.supportArray() looks like it is needed for answering questions about function parameters which are probably resolvable by the existing FunctionMetadata. Similar problems with ArrayMode - it's probably providing an incomplete solution to something that can perhaps be solved better using proper RVA token classification. Please review the re-worked patch and address these issues as best you can. -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug. --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
