Bug 82577 - get rid of prex.h / postx.h wrapper headers
Summary: get rid of prex.h / postx.h wrapper headers
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: Other Linux (All)
: medium normal
Assignee: Jorenz Paragas
URL:
Whiteboard: ToBeReviewed target:5.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-08-13 20:06 UTC by Michael Stahl (CIB)
Modified: 2017-02-14 08:57 UTC (History)
3 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 (CIB) 2014-08-13 20:06:22 UTC
includes of X11 headers in LO are wrapped with
#include <prex.h>
and
#include <postx.h>

this is because old LO code, especially in "tools" and "vcl",
defines types that have the same names as X11 types,
so the headers do a bunch of stupid macro hackery
to rename the X11 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 X11 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 X11 type
   and the LO type visible in the same LO source file
   by not including both the defining LO and X11 headers

3) if the X11 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 X11 type is a macro, then adding a namespace
   prefix to the LO type is probably required
Comment 1 Noel Grandin 2014-08-14 07:08:12 UTC
Is there a compiler flag that we can turn on that will automatically report type name collisions?
Comment 2 Björn Michaelsen 2014-08-14 08:46:47 UTC
(In reply to comment #1)
> Is there a compiler flag that we can turn on that will automatically report
> type name collisions?

Even easier: New type names should be in a namespace. That should make practically rule out collisions of types (up until we collide a full namespace somewhere).
Comment 3 Tor Lillqvist 2014-08-17 07:15:18 UTC
One thing that we need consensus on is whether to use a "real" C++ namespace or a "C-style" one, i.e. just prefixing the conlicting LibreOffice type names with some short string, like "Vcl".

I guess the C++ namespace would be better from a C++ orthodoxy point of view, but what should the namespace be? "vcl::" ? "org::libreoffice::vcl::" (brrr)? In any case, we *don't* want to repeat the current disaster of inconsistent "using" declaration, varying from one source file to another. Would using a C++ namespace have the benefit that it would be enough to just add a "using namespace vcl" (or whatever) in some header, and only those few source files that actually refer to the identically-named X11 types would need to add a :: prefix to those?

Personally I wouldn't mind using a "C-style" prefix, but then I am well known to not really be that huge a C++ fan.

For the cases where there are clashes with *macros* (I guess mostly for Win32?), the "C-style" identifier prefix (or even renaming our identifier completely) is the only solution, right?
Comment 4 Björn Michaelsen 2014-08-18 09:04:29 UTC
(In reply to comment #3)
> One thing that we need consensus on is whether to use a "real" C++ namespace
> or a "C-style" one, i.e. just prefixing the conlicting LibreOffice type
> names with some short string, like "Vcl".

IHMO C++ namespaces as its better for tooling, e.g. doxygen:

 http://docs.libreoffice.org/sw/html/namespaces.html
 
> I guess the C++ namespace would be better from a C++ orthodoxy point of
> view, but what should the namespace be? "vcl::" ? "org::libreoffice::vcl::"
> (brrr)?

No, not a java-like "lets prefix my full home address" thing. We already have "com::sun::star::" as our public API. There were discussions of aliasing that as "libreoffice::" (llunak wanted something like that IIRC).

For our private/internal API, having "vcl::Foo" should already be less risky than "VclFoo" -- and in the long term, if needed, we could move "vcl::" to something-like "libreoffice::private::vcl::" or "libreoffice::internal::vcl::", when there are conflicts. It will be a lot easier to move a namespace than to rename hundreds of classes.

> In any case, we *don't* want to repeat the current disaster of
> inconsistent "using" declaration, varying from one source file to another.

IMHO that is mostly a symptom of having  unmanageable 10KLOC or more per source file, so that its not easy to see what a using (or #include for the matter) is for and thus they are rarely touched and just grow.

> Would using a C++ namespace have the benefit that it would be enough to just
> add a "using namespace vcl" (or whatever) in some header, and only those few
> source files that actually refer to the identically-named X11 types would
> need to add a :: prefix to those?

using in header files is so harmful that most coding styles disencourage it, e.g.  Sutter/Alexandrescus "C++ Coding Standards" rule 59: "Don’t write namespace usings in a header file or before an #include."
http://stackoverflow.com/questions/5849457/using-namespace-in-c-headers

Anyway, this is more of a discussion for the mailing list or the ESC, lets take it there if there is further need.
Comment 5 Björn Michaelsen 2014-08-18 09:06:53 UTC
Oh, one more thing: Making everything C++ mangled names (or other long prefixes) might have an impact on dynamic linking and thus startup time. mmeeks would have some words about that, Im sure.
Comment 6 Commit Notification 2014-08-23 17:32:34 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577, fdo#82579: Handle Cursor



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 7 Commit Notification 2014-08-23 21:13:57 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle KeyCode



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 8 Commit Notification 2014-09-18 06:57:32 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle Font



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 9 Stephan Bergmann 2014-09-23 07:35:38 UTC
see <http://lists.freedesktop.org/archives/libreoffice/2014-September/063545.html> "Re: Performance samples for LibreOffice ..." for another benefit of fixing this fully
Comment 10 Commit Notification 2014-09-23 11:15:05 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle Window



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 11 Commit Notification 2014-09-30 08:00:36 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle Region



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 12 Commit Notification 2014-09-30 09:50:03 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle PolyPolygon



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 13 Commit Notification 2014-10-01 07:36:19 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle Time



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 14 Commit Notification 2014-10-01 09:00:46 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle Icon



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 15 Commit Notification 2014-10-01 10:22:23 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82577: Handle KeyPress



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 16 Robinson Tryon (qubit) 2015-12-14 04:59:02 UTC Comment hidden (obsolete)
Comment 17 Jorenz Paragas 2016-01-24 20:49:43 UTC
I'm going to work on this task.
Comment 18 Commit Notification 2016-01-25 09:55:17 UTC
Jorenz Paragas committed a patch related to this issue.
It has been pushed to "master":

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

tdf#82577: Handle DestroyAll, InitializeToken, NextRequest

It will be available in 5.2.0.

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 19 Commit Notification 2016-01-30 07:47:15 UTC
Jorenz Paragas committed a patch related to this issue.
It has been pushed to "master":

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

tdf#82577: Remove remaining #undefs in postx.h

It will be available in 5.2.0.

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 20 Robinson Tryon (qubit) 2016-02-18 14:52:16 UTC Comment hidden (obsolete)
Comment 21 Commit Notification 2016-02-22 20:35:34 UTC
Jorenz Paragas committed a patch related to this issue.
It has been pushed to "master":

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

tdf#82577: Remove prex.h and postx.h wrapper headers

It will be available in 5.2.0.

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 22 Jorenz Paragas 2016-02-23 06:29:02 UTC
With the above commit, this task is finished.