Apache OpenOffice (AOO) Bugzilla – Issue 85898
Remove all external header guards
Last modified: 2008-05-13 05:25:41 UTC
See $subject. The optimization they are there for is nowadays built into compilers. Therefore, removing this cruft (which in fact poses quite some subtle fragility).
Set a target.
Doing this has the potential for quite a lot conflicts in CWS' which exist in parallel to CWS incguards01: Every change in the include list of a file, in any CWS, will cause a conflict when integrated or resynced. I *strongly* vote for not doing this without a *really* good reason ("some subtle fragility" is *not* a good reason, sorry). We once had an agreement that the goal is a good one, but that we won't do this with a single large change - for good reasons. Please let's keep this agreement. Please let the decision when to remove the include guards to the module/source owners, instead of patronizing them. For instance, I clean up include guards in any file I work in, as long as I can be halfway sure that this will cause no conflict (because the file is strictly mine, and no parallel CWS also touching it exists). I do not have the smallest desire to deal with all the conflicts you will introduce with this change. Don't do it, please!
fs: Most of the work is done by a script, so avoiding conflicts is in fact easy - just before the integration run the script on files that were changed in the CWS, and that's it. I can easily extend it to detect changed files in the CWS, and operate just on them.
kendy: well, not completely: You know what I currently quite often do in files I touch? Remove the include guards, put the includes in a certain order and remove the ones which obviously are not needed anymore (yes, medics call this anankasm :), and add some markers which allow some newer IDE macros of mine to automatically add new includes below those markers, for the identifier under the cursor. Don't tell me this doesn't lead to conflicts :) Not sure if extending the script to work on CWS-modified files only is needed - my idea would be to merely run it on selected modules in a selected CWS just before it's handed over to QA. I'd say this would leave sufficient control over where and when to apply. But thanks for the offering.
fs: Sure - when you remove the include guards by hand, for sure there are going to be conflicts :-( Wrt. running on selected modules in selected CWS's before handing over to QA, I think we mean mostly the same, maybe I just did not explain myself well enough ;-) Either way, I'll be glad to help to do the transition as smooth as possible. The thing (for me) is - generally I am not a friend of big-bang changes, but in this case I think it is the best we can do, because the task is easily scriptable. As the result we'll have much better readable code everywhere, not just where there was a hacker kind enough to follow the advice in the OOo coding standards to remove the guards in the files he/she touches.
Well, as I wrote in a PM to Thorsten, I am absolutely not convinced that the headache we get with the big bang (resync conflicts, resync times, build problems for wrongly resolved resync conflicts) is worth the gain. Especially when you say that the gain is "much better readable code", I tend to disagree here - Personally, I seldom "read" the part of the file containing the includes, so from this perspective, I don't see a gain. Of course the include guards look, hmm, old-fashioned, but fixing this is, taken alone, not worth the pain. Anyway.
@fs: I see your point, with your specific module (where, as you told, largely reworked the include sections, and even removed guards). I don't see it across the board. This section of files is only seldomly touched - but having inconsistent or even external-lib-dependent header guards is fragile, the code repels outside hackers, it violates a rule of the coding standard - do I need more arguments to justify my wanting this cleaned up? The only thing a few devs need to do is solve a some conflicts manually (and these are _easy_ conflicts, mind that).
Done, as announced on dev@ooo (_including_ dbaccess).
I can verify (by random inspection) that include guards have been removed from source files. I also verify that the beast still builds (tested successfully on windows, linux, and solaris x86). Helge, assigning this issue to you for verification by automatic tests that there is no functionality broken.
HDE: required automated tests successfully passed on CWS
HDE: OK in Master