[GitHub] [openoffice] ardovm opened a new pull request #89: Use std::vector instead of fixed-size array of cffLocal objects

[GitBox]

ardovm opened a new pull request #89:
URL: https://github.com/apache/openoffice/pull/89


   This is a follow-up of [bug 125359](https://bz.apache.org/ooo/show_bug.cgi?id=125359).
   
   Instead of using fixed-size arrays, use `std::vector` as a variable-sized array for `CffLocal` objects.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652464308


   I see that in the same file there are other fixed-size arrays used as stacks (if I understand correctly):
   * `CffSubsetterContext::mnValStack`,
   * `CffSubsetterContext::mnHintStack`.
   
   If it is of any interest to turn also them into `std::vector` or other containers, please let me know, and I could include them into this PR.
   
   Please be aware that this is my first PR against OpenOffice; if there are any tests I can run to validate these changes, kindly give me some pointers. By now, I just built the source and tried to reproduce the crash originating [bug 12359](https://bz.apache.org/ooo/show_bug.cgi?id=125359).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652654917


   If you ncould change those and add them to the PR would be great. Or maybe make a new PR. I have a look on the weekend.
   
   I try to get some pointers on testing. I struggle there myself.
   
   all the best
   Petko


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-652807797


   Thank you, Petko. It would help me to know what is the expected average size and variation of such stacks, to give a "meaningful" initial size and optimize the number of calls to `std::vector::resize()`.
   
   My real problem is, in fact, that I have no idea about the purpose of that code! For example, what is a CFF? And where is that code called from? (I could do a full-text search of the sources, but some more structured information would help).
   
   I like documenting my own code and I would be glad to help also in this way, at least in this file we are working on. Are there any coding standards about code documentation in OpenOffice.org? For example, are you adopting Doxygen, JavaDoc or the like?
   
   Thank you in advance and best regards.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

Pilot-Pirx commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-653749584


   As a first test I could build AOO42X on Windows with this patch applied.
   I could also successfully export a document to PDF with Noto Sans CJK font.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-653869909


   @ardovm
   I searched a bit for CFF on our opengrok site ( http://opengrok.openoffice.org/search?project=trunk&full=CFF&defs=&refs=&path=&hist=&type=&si=full ). And it looks like this is something to do with Fonts. Searching for fonts and CFF brought up this explanation:
   https://www.linotype.com/8120/the-difference-between-cff-and-ttf.html
   
   I am not sure. But there is a cff.cxx file which has some fixed Array. I guess that they are linked.
   HTH. I will check later for further analysis if I can dig more Information out.
   
   Sorry that we have to do a bit of R&D and I can not give you a steight answer.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-657786860


   I am trying to get a grasp on the CFF concepts. It's... a whole world. Thank you @leginee for the pointers!
   
   However it will take me some more time to understand if it is worth changing also those "stacks" to `std::vector`'s... I hope you are not in a hurry :-)
   
   In the meantime, I took the liberty to add some comments. Feel free to criticize the style.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-657991972


   I like the comments. 👍
   Take the time you need. We are all on the same team.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662696345


   I started to dig a bit deeper into CFF and Type2, then I understood they are out of my reach.
   In the end, if the code works well as it is, I would refrain to change it any more than this.
   
   Here is what I understood about the other fixed-size arrays: `CffSubsetterContext::mnValStack`, `CffSubsetterContext::mnTransVals` and `CffSubsetterContext::mnHintStack`.
   They are sized as per the Type 2 Charstring Format specifications, appendix B.
   They are often accessed by index, i.e. not only by push and pop operations.
   Are we interested in adding run-time checks on their boundaries? Limits should not be crossed by compliant fonts.
   
   If we wanted to minimize the size of objects, we could move them to the heap using fixed-size `std::vector`, but I am not sure we need this.
   
   Please note that I partially reverted one commit. I could force-push and merge everything into a single commit to make merging simpler.
   
   I am waiting for indications, on whether and how to proceed any further.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662838003


   Thanks for the feedback. I think the patch is then feature complete.
   To have one commit that can be simple be merged would be nice. Please do the merge.
   
   I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   
   > Are we interested in adding run-time checks on their boundaries?
   can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   I try to review next week.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

leginee edited a comment on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662838003


   Thanks for the feedback. I think the patch is then feature complete.
   To have one commit that can be simple be merged would be nice. Please do the merge.
   
   I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   
   > Are we interested in adding run-time checks on their boundaries?
   
   can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   I try to review next week.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

[GitBox]

ardovm commented on pull request #89:
URL: https://github.com/apache/openoffice/pull/89#issuecomment-662862458


   > Thanks for the feedback. I think the patch is then feature complete.
   > To have one commit that can be simple be merged would be nice. Please do the merge.
   
   Done.
   
   > I am not sure if there is some unit test around. I will check on this when I review. But it would be preferable if we could add a unit test that checks the behavior.
   >
   > > Are we interested in adding run-time checks on their boundaries?
   >
   > can we create a test scenario that may break the boundaries? - If we have a unit test that manages to break the code I say yes we should implement checks for that, that are efficient as possible.
   
   If I understand correctly the matter, we should test the code against actual CFF fonts.
   I would not know where to start.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]