Apache OpenOffice (AOO) Bugzilla – Issue 73468
OpenOffice++
Last modified: 2017-05-20 11:31:34 UTC
During the course of the joint software quality improvement project OpenOffice++ of MultiRacio Ltd. and the Department of Software Engineering, University of Szeged, a number of patches were created for OpenOffice.org. These patches aim to improve the general quality of the OpenOffice.org source code and are not targeted at any particular component (that's why I've picked "hu" for now, but please move the issue as appropriate). They were created by automatically sweeping the source code for suspect parts of code and then manually investigating these parts and providing a patch where it made sense. The patches don't necessarily "fix" bugs, but they may very well prevent them from creeping in later. For a detailed technical report on the project and further reading please refer to its home page at http://oopp.multiracio.com/ I am going to attach the patches from the different phases of the project in (slightly) different formats. If you prefer one format over the other please get in touch and we will make every effort to cooperate in the integration of these patches. You can reach me personally at darabos.daniel@multiracio.hu, but we are going to respond to comments to this issue as well. This work was supported by the EU funded Hungarian grant GVOP-3.1.1.-2004-05-0345/3.0.
Created attachment 42152 [details] Patches from Phase 1 (against 1.1.5)
Created attachment 42153 [details] Patches from Phase 2 (against 2.0.4)
I'm tentatively changing the component to "porting", because "hu" was really not a very good choice (the patches have nothing to do with any language but C++). Sorry.
I forgot to reassign the issue to the owner of porting (I guess that's Martin, right?). Sorry for fiddling so much with the issue...
Looking at diffs.2.zip: 1 Evaluating the patches would be easier with some short information why these changes are considered necessary. 2 Since OOo 2.1, most C/C++ code is warning free and warnings are treated as errors (although unfortunately this is not the default behavior and has to be enabled with --enable-werror). I assume that some of your patches address issues that have already been solved when making the code warning free. GEN1007.diff: I assume these changes are necessary so that an object member is not used before it is initialized (at least the last two diffs are obviously of that kind, I did not check the others). This is a good change, yes. However, in some cases initializer lists in constructors will also have to be changed, to avoid warnings (= errors); for example, moving up m_aScrollSB in hangulhanjadlg.hxx needs to be reflected in the corresponding hangulhanjadlg.cxx. GEN7052.diff: I assume these changes are necessary to calculate double = int op double instead of double = int op int which are probably good changes (somebody knowing those code places should have a look, in case the truncation *was* intended). GEN7053.diff: For one, most of those places are probably better fixed by simply dropping the L suffix. For another, please do not introduce any new C style casts in C++ code. And in the patch to binfilter/bf_forms/source/component/forms_imgprod.cxx various lines lack closing parentheses, which looks very suspicious.
thank you for the patches. To make review possible in a short time frame it would help if the patches are filed on per projects basis (Impress, Calc, Writer, framework, etc.) and attach a short description of the patches being made.
Sorry, I was in a bit of a hurry yesterday and forgot to include the descriptions of the individual types of patches. So here are the definitions as reported by Colombus, the source code analyzer of the University of Szeged: GEN1007: data member is being initialized by a non-initialized data member (data members in the constructor initializer list are being initialized in the same order as they were declared in the class body) GEN7052: in case of storing the result of a division in a float/double variable, the operands should be casted to float/double to avoid rounding errors GEN7053: long integer literals should be casted to sal_Int32 when participating in an expression whose result's type is sal_Int32 I'm from the Multiracio Ltd. side of the project, so I'll try to get someone from Szeged here to answer questions about these issues. In particular I had my reservations about the casts in GEN7053, and that patch was created in the last minute -- this can be seen as the cause of the missing parentheses. I'll be attaching a fixed patched in a moment. The types of rule violations found in Phase 1 (in 1.1.5) are the following: GEN1004: classes that use dynamically allocated memory should have a copy constructor, an assignment operator and a destructor GEN1020: data members should be initialized GEN7035: switch instructions of integer type should have a 'default' label GEN7051: static variables should be declared as const We would of course like to recreate these patches for 2.1 (and seeing what got fixed already would be valuable feedback), and we would also like to be able to create an online source code monitoring system for OpenOffice.org (like the one currently available for Mozilla at http://frontendart.com/momo.php). I'm not sure about the organization of the project, but let's hope that we can secure the resources to do these. Thank you for looking at our patches. I will be reorganizing them by component affected and refiling them during the week.
Created attachment 42169 [details] fixed patch for Phase 2
added myself to cc list
Created attachment 45158 [details] Patches in unified format updated for 2.2
Hi, sorry about taking so long! We have now updated the patches for OpenOffice.org 2.2 (it was a pleasant surprise to see that a number of the patches became invalid because they got fixed!) and I have cleaned up their formatting. I have also split this file to small files by code module and I am going to file them as separate issues now so that they get to where they belong. Thank you for your patience and I hope we can help better OOo. We have also received some very valid and useful comments regarding these patches from Nikolai Pretzell, and we are (the guys at University Szeged are) preparing a new analysis based on the lessons learned.
I have created the project specific issues. My apologies for spamming :).
so patches are dispatched to subissues, reseting this issue to task for tracking the subtasks.
Reset assigne to the default "issues@openoffice.apache.org".