Bug 82579 - get rid of premac.h / postmac.h wrapper headers
Summary: get rid of premac.h / postmac.h wrapper headers
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: Other Mac OS X (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-08-13 20:11 UTC by Michael Stahl
Modified: 2017-02-14 08:57 UTC (History)
2 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Stahl 2014-08-13 20:11:35 UTC
includes of Mac OS X headers in LO are wrapped with
#include <premac.h>
and
#include <postmac.h>

this is because old LO code, especially in "tools" and "vcl",
defines types that have the same names as Mac OS X types,
so the headers do a bunch of stupid macro hackery
to rename the Mac OS X types so they don't collide with LO types.

it would be obviously much simpler if the LO types
simply had names that don't collide with Mac OS X types.

"git grep" or ctags or opengrok.libreoffice.org
should find the definitions of types in LO that are re-defined
in the wrapper headers.

there are several ways to clean up collisions:

1) if there is no equivalent in LO of the redefined type,
   it can just be removed from the wrapper header

2) in some cases it can be avoided to have the Mac OS X type
   and the LO type visible in the same LO source file
   by not including both the defining LO and Mac OS X headers

3) if the Mac OS X type is not a macro, then putting the LO
   type into a namespace and namespace-qualifying the uses
   should avoid the collisions

4) if the Mac OS X type is a macro, then adding a namespace-like
   prefix to the LO type is probably required
Comment 1 Commit Notification 2014-08-17 10:14:48 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c2d3b7e4aa0480ed2c66b7240ef05cfff6a07125

fdo#82579: Only Point, Polygon, Size and TimeValue need to be handled



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 2 Commit Notification 2014-08-17 10:31:20 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=95a5d7733190d66b1c4761a5bbaa5a2cc2361dd0

fdo#82579: The SDK used by the "TDF" tb needs Cursor and Line to be handled



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 3 Tor Lillqvist 2014-10-05 08:46:28 UTC
I guess handling TimeValue will be problematic here, as it is defined in a stable API header, <osl/time.h>.

OK to break API (and ABI?) by renaming TimeValue there to OslTimeValue?

Or is something more complicated needed to hide the osl TimeValue in those (relatively few?) source files that include both the <osl/time.h> and Mac and iOS system headers? A start of an attempt at that would be as below:

diff --git a/include/postmac.h b/include/postmac.h
index a0cc11b..b489d67 100644
--- a/include/postmac.h
+++ b/include/postmac.h
@@ -21,6 +21,5 @@
 #undef Point
 #undef Polygon
 #undef Size
-#undef TimeValue
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/include/premac.h b/include/premac.h
index f22c193..ee201c9 100644
--- a/include/premac.h
+++ b/include/premac.h
@@ -21,6 +21,5 @@
 #define Point MacOSPoint
 #define Polygon MacOSPolygon
 #define Size MacOSSize
-#define TimeValue MacOSTimeValue
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/vcl/inc/osx/salinst.h b/vcl/inc/osx/salinst.h
index 8a88a26..2a3032d 100644
--- a/vcl/inc/osx/salinst.h
+++ b/vcl/inc/osx/salinst.h
@@ -20,6 +20,8 @@
 #ifndef INCLUDED_VCL_INC_OSX_SALINST_H
 #define INCLUDED_VCL_INC_OSX_SALINST_H
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "comphelper/solarmutex.hxx"
 #include "osl/thread.hxx"
 #include "osl/conditn.h"
diff --git a/vcl/osx/a11ycomponentwrapper.mm b/vcl/osx/a11ycomponentwrapper.mm
index b91029d..5a2d52a 100644
--- a/vcl/osx/a11ycomponentwrapper.mm
+++ b/vcl/osx/a11ycomponentwrapper.mm
@@ -17,6 +17,8 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "quartz/utils.h"
 #include "a11ycomponentwrapper.h"
 #include "a11yrolehelper.h"
diff --git a/vcl/osx/a11yfactory.mm b/vcl/osx/a11yfactory.mm
index 30437a8..3437565 100644
--- a/vcl/osx/a11yfactory.mm
+++ b/vcl/osx/a11yfactory.mm
@@ -17,7 +17,6 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
-
 #include "osx/salinst.h"
 #include "osx/a11yfactory.h"
 #include "osx/a11yfocustracker.hxx"
diff --git a/vcl/osx/a11yrolehelper.mm b/vcl/osx/a11yrolehelper.mm
index 45423db..2e1aee3 100644
--- a/vcl/osx/a11yrolehelper.mm
+++ b/vcl/osx/a11yrolehelper.mm
@@ -17,6 +17,7 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
 
 #include "osx/a11yfactory.h"
 
diff --git a/vcl/osx/printaccessoryview.mm b/vcl/osx/printaccessoryview.mm
index e2db4c3..35fc28a 100644
--- a/vcl/osx/printaccessoryview.mm
+++ b/vcl/osx/printaccessoryview.mm
@@ -19,6 +19,8 @@
 
 #include "sal/config.h"
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "tools/resary.hxx"
 
 #include "vcl/print.hxx"
diff --git a/vcl/osx/printview.mm b/vcl/osx/printview.mm
index 8b324b9..498e9ed 100644
--- a/vcl/osx/printview.mm
+++ b/vcl/osx/printview.mm
@@ -17,6 +17,7 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
 
 #include "vcl/print.hxx"
 
diff --git a/vcl/osx/saldata.cxx b/vcl/osx/saldata.cxx
index f4dc179..0a94673 100644
--- a/vcl/osx/saldata.cxx
+++ b/vcl/osx/saldata.cxx
@@ -19,6 +19,8 @@
 
 #include <config_features.h>
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "osx/saldata.hxx"
 #include "osx/salnsmenu.h"
 #include "osx/salinst.h"
diff --git a/vcl/osx/salframeview.mm b/vcl/osx/salframeview.mm
index 8e74086..fbedacf 100644
--- a/vcl/osx/salframeview.mm
+++ b/vcl/osx/salframeview.mm
@@ -17,6 +17,8 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include <sal/alloca.h>
 #include <sal/macros.h>
 
diff --git a/vcl/osx/salinst.cxx b/vcl/osx/salinst.cxx
index d8adf03..8223214 100644
--- a/vcl/osx/salinst.cxx
+++ b/vcl/osx/salinst.cxx
@@ -21,6 +21,8 @@
 
 #include <stdio.h>
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include <tools/solarmutex.hxx>
 
 #include "osl/process.h"
@@ -665,7 +667,7 @@ void AquaSalInstance::Yield( bool bWait, bool bHandleAllCurrentEvents )
         // wait until any thread (most likely the main thread)
         // has dispatched an event, cop out at 200 ms
         osl_resetCondition( maWaitingYieldCond );
-        TimeValue aVal = { 0, 200000000 };
+        OslTimeValue aVal = { 0, 200000000 };
         sal_uLong nCount = ReleaseYieldMutex();
         osl_waitCondition( maWaitingYieldCond, &aVal );
         AcquireYieldMutex( nCount );
diff --git a/vcl/osx/salsys.cxx b/vcl/osx/salsys.cxx
index f10ba61..b75d249 100644
--- a/vcl/osx/salsys.cxx
+++ b/vcl/osx/salsys.cxx
@@ -17,6 +17,8 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "rtl/ustrbuf.hxx"
 
 #include "vcl/button.hxx"
diff --git a/vcl/osx/saltimer.cxx b/vcl/osx/saltimer.cxx
index 17c4d80..ff3fd49 100644
--- a/vcl/osx/saltimer.cxx
+++ b/vcl/osx/saltimer.cxx
@@ -17,6 +17,8 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "osx/saltimer.h"
 #include "osx/salnstimer.h"
 #include "osx/saldata.hxx"
diff --git a/vcl/osx/vclnsapp.mm b/vcl/osx/vclnsapp.mm
index d1d086f..4955ae2 100644
--- a/vcl/osx/vclnsapp.mm
+++ b/vcl/osx/vclnsapp.mm
@@ -19,6 +19,8 @@
 
 #include <config_features.h>
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include "sal/config.h"
 
 #include <vector>
diff --git a/vcl/quartz/ctlayout.cxx b/vcl/quartz/ctlayout.cxx
index cd9771a..de076e6 100644
--- a/vcl/quartz/ctlayout.cxx
+++ b/vcl/quartz/ctlayout.cxx
@@ -17,6 +17,8 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include "quartz/hide-osl-timevalue.h"
+
 #include <sal/types.h>
 #include <boost/ptr_container/ptr_vector.hpp>
 #include "tools/debug.hxx"
diff --git a/vcl/quartz/salgdi.cxx b/vcl/quartz/salgdi.cxx
index 80d6d5e..7130f7d 100644
--- a/vcl/quartz/salgdi.cxx
+++ b/vcl/quartz/salgdi.cxx
@@ -21,10 +21,7 @@
 
 #include "sal/config.h"
 
-#include "osl/file.hxx"
-#include "osl/process.h"
-
-#include "osl/mutex.hxx"
+#include "quartz/hide-osl-timevalue.h"
 
 #include "rtl/bootstrap.h"
 #include "rtl/strbuf.hxx"
diff --git a/vcl/source/app/dbggui.cxx b/vcl/source/app/dbggui.cxx
index 85b4528..d32d61e 100644
--- a/vcl/source/app/dbggui.cxx
+++ b/vcl/source/app/dbggui.cxx
@@ -52,7 +52,6 @@
 #include "vcl/unohelp2.hxx"
 
 #include "salinst.hxx"
-#include "svsys.h"
 
 #include "com/sun/star/i18n/XCharacterClassification.hpp"
 
diff --git a/vcl/source/app/settings.cxx b/vcl/source/app/settings.cxx
index a30fd34..90015f6 100644
--- a/vcl/source/app/settings.cxx
+++ b/vcl/source/app/settings.cxx
@@ -21,7 +21,6 @@
 
 #include <officecfg/Office/Common.hxx>
 
-#include <svsys.h>
 #include "comphelper/processfactory.hxx"
 #include <rtl/bootstrap.hxx>
 #include "tools/debug.hxx"
diff --git a/vcl/source/window/accessibility.cxx b/vcl/source/window/accessibility.cxx
index 0f3b416..720dd4f 100644
--- a/vcl/source/window/accessibility.cxx
+++ b/vcl/source/window/accessibility.cxx
@@ -54,10 +54,6 @@
 #include "vcl/virdev.hxx"
 #include "vcl/settings.hxx"
 
-// declare system types in sysdata.hxx
-#include "svsys.h"
-#include "vcl/sysdata.hxx"
-
 #include "salframe.hxx"
 #include "salobj.hxx"
 #include "salinst.hxx"
diff --git a/vcl/source/window/syschild.cxx b/vcl/source/window/syschild.cxx
index 78c31f1..d930c3d 100644
--- a/vcl/source/window/syschild.cxx
+++ b/vcl/source/window/syschild.cxx
@@ -24,9 +24,6 @@
 
 #include <tools/rc.h>
 
-// declare system types in sysdata.hxx
-#include <svsys.h>
-
 #include <vcl/window.hxx>
 #include <vcl/sysdata.hxx>
 #include <vcl/svapp.hxx>
Comment 4 Tor Lillqvist 2014-10-06 08:41:11 UTC
Forgot the quartz/hide-osl-timevalue.h, would be something like this. (But the above was just one test approach, not sure at all that it is sane.)

#define TimeValue OslTimeValue
#include <osl/conditn.h>
#include <osl/file.hxx>
#include <osl/mutex.hxx>
#include <osl/process.h>
#include <osl/thread.hxx>
#include <osl/time.h>
#undef TimeValue
Comment 5 Stephan Bergmann 2014-10-06 08:50:02 UTC
(In reply to Tor Lillqvist from comment #3)
> I guess handling TimeValue will be problematic here, as it is defined in a
> stable API header, <osl/time.h>.
> 
> OK to break API (and ABI?) by renaming TimeValue there to OslTimeValue?

Even though certainly very poor choice of non-prefixed name in osl/time.h, it would be better if we can work around the issue without breaking API (as outlined in your patch).
Comment 6 Tor Lillqvist 2014-10-06 08:53:14 UTC
Or we could just say that having premac.h/postmac.h is not that bad after all and close this bug as WONTFIX. After all, the above approach at least is hardly any cleaner...
Comment 7 Fernando Montes 2015-03-23 21:53:55 UTC
Well i am going to try to do what is left of this. Hopefully i can get ride of both headers. Though i doubt getting rid of them will improve performance by much.
Comment 8 Tor Lillqvist 2015-03-23 21:57:49 UTC
What made you think this bug has anything to do with performance?
Comment 9 Fernando Montes 2015-03-23 22:04:31 UTC
I don't know. I don't really understand what the bug really is. I understand its about types conflicting with each other. But much like you said I can't think of a clear way to do it not using a wrapper. I even talked to my professor about it. The only thing he could think of was making a namespace for the conflicting types. But would that be any cleaner then headers?
Comment 10 Robinson Tryon (qubit) 2015-12-14 04:59:03 UTC Comment hidden (obsolete)
Comment 11 Robinson Tryon (qubit) 2016-02-18 14:51:43 UTC Comment hidden (obsolete)
Comment 12 jani 2016-08-05 09:17:11 UTC
Please be aware, that this easyhack is considered an important but large scale cosmetic change as described in https://wiki.documentfoundation.org/Development/LargeScaleChanges

It was in decided by the ESC to close this kind of easyhacks, and send them directly as mail, to new contributors.
https://lists.freedesktop.org/archives/libreoffice/2016-August/074920.html

Please do not submit patches with many files !!

This particular easyhack is kept open as an exception to the rule.