I provided a detailed comment on Vincent's brief review at: https://issues.apache.org/bugzilla/show_bug.cgi?id=49687#c31
With the exception of the the following comment, the remaining comments are editorial in nature or have no actionable response. > How feasible is it to run the BIDI algorithm on a subset of the FO tree? If > I'm > right, ATM it is launched on a whole page-sequence. When we refactor the code > so that layout can be started before a whole page-sequence has been parsed, in > order to avoid the infamous memory issue that a lot of users are running into, > will that code allow to do it? Regarding the point(s) at which the bidi algorithm is invoked, I agree there are possible, alternative points of invocation. Some of which may distribute the cost of its performance as opposed to concentrating that work in one call. However, even if there are alternatives, it is not clear whether they are desirable, and whether it would have any impact on memory usage or time performance. Further, "when we refactor the code" is an unknown, future possibility, and making a substantial change at this time in order to perform certain optimizations that only come into play in the eventuality that such code refactoring occurs seems to be an example of premature optimization. As regards to symbol name length, I would merely cite that the FOP Coding Conventions <http://xmlgraphics.apache.org/fop/dev/conventions.html> do not specify length of an identifier (either short or long) as an established convention. Let's not use this valuable contribution as a *cause célèbre* to argue or establish new conventions that do not exist. Otherwise we will soon be arguing over whether i, j, and k are reasonable identifiers for enumeration variables. You ask about whether this merge is truly production ready? Is this a requirement for doing a merge from a temporary branch? How would you define truly production ready? Is FOP itself truly production ready? I've spend the last couple of months creating over 500,000 tests. I wonder if any other part of FOP is so thoroughly tested? Frankly speaking, I would like to put even more tests into place, and I will over time, but I'm not going to do that until the current work is merged. If there is a substantial bug or regression that would be caused by this merger, then I'd be happy to address that problem before merging. I look forward to any report of this nature. If folks wish to keep pushing the issue of symbol name length, then please enumerate, by source file and line number each case to which is objected. Also, please provide objective comparative data showing how the reported data diverges from statistically similar usage elsewhere in FOP. Please also define an objective measure of the level of domain knowledge needed for working with specific code in this merge for which a comment about symbol length is an issue. In particular, please show how a reader skilled in the arts of the code being considered would not be able to readily infer the meaning of a specific symbol in cases where I have not explicitly provided a comment at the place of definition. G. On Wed, Oct 19, 2011 at 4:50 PM, Chris Bowditch <bowditch_ch...@hotmail.com>wrote: > On 18/10/2011 19:55, Simon Pepping wrote: > >> I merged the ComplexScripts branch into trunk. Result: >> > > Hi Simon, > > As well of the question of how to do the merge there is also the question > should we do the merge? Of course this is a valuable feature to the > community, and Glenn has invested a lot of time in its development but is it > truely production ready? I asked Vincent to take a look at the branch > earlier in the year as it's a feature we also need, but he had several > concerns that have not be adequately answered. Take a look at comment #30; > https://issues.apache.org/**bugzilla/show_bug.cgi?id=**49687#c30<https://issues.apache.org/bugzilla/show_bug.cgi?id=49687#c30> > > I'm not sure why Vincent describes it as a "brief look" because he spent > several days on it. I also asked Peter to take a look and he had similar > concerns. 2 or 3 letter variable names are a barrier for any committer > wanting to maintain this code and I don't think it is a sufficient argument > to say that a pre-requisite to maintaining this code is to be a domain > expert. I would hope that any experienced committer with a debugger should > be able to solve some bugs. Obviously certain problems will require domain > expertise, but the variables names are a key barrier to being able to > maintain this code. > > I realise my comments might be a little controversial and I don't mean any > disrespect to Glenn or his work (which is largely excellent), but we should > at least discuss these topics before the merge is completed. > > Thanks > > Chris > > > >> --- Merging r981451 through r1185769 into '.': >> >> Summary of conflicts: >> Text conflicts: 58 >> Tree conflicts: 126 >> >> Most tree conflicts are probably an artifact of subversion. See >> >>> svn info lib/xmlgraphics-commons-1.**5svn.jar|tail -n 4 >>> >> Tree conflict: local add, incoming add upon merge >> Source left: (file) https://svn.apache.org/repos/** >> asf/xmlgraphics/fop/trunk/lib/**xmlgraphics-commons-1.5svn.**jar@981450<https://svn.apache.org/repos/asf/xmlgraphics/fop/trunk/lib/xmlgraphics-commons-1.5svn.jar@981450> >> Source right: (file) https://svn.apache.org/repos/** >> asf/xmlgraphics/fop/branches/**Temp_ComplexScripts/lib/** >> xmlgraphics-commons-1.5svn.**jar@1185769<https://svn.apache.org/repos/asf/xmlgraphics/fop/branches/Temp_ComplexScripts/lib/xmlgraphics-commons-1.5svn.jar@1185769> >> >> This will cause quite some work. >> >> I also merged trunk into ComplexScripts. Result: >> >> --- Merging r1177231 through r1185780 into '.': >> >> Summary of conflicts: >> Text conflicts: 2 >> Tree conflicts: 2 >> >> I resolved the text conflicts easily. Again the tree conflicts were >> not real conflicts. >> >> Both merges should result in the same code: trunk + ComplexScripts. >> >> I did not commit the merge of trunk into ComplexScripts to the >> repository. I do not think it would facilitate merging ComplexScripts >> into trunk. >> >> Simon >> >> On Sat, Oct 15, 2011 at 06:17:49PM +0800, Glenn Adams wrote: >> >>> With this latest patch, I am satisfied that there is sufficient testing >>> and >>> stability in the CS branch to support its merger into trunk. Therefore, I >>> request that such a merge be accomplished after applying patch 5 to the >>> CS >>> branch as described below. >>> >> >> >
