Bug Hunting Session
Bug 65108 - Clean-up header includes (global/local)
Summary: Clean-up header includes (global/local)
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.2.0.0.alpha0+ Master
Hardware: Other All
: medium enhancement
Assignee: Feyza
URL:
Whiteboard: ToBeReviewed
Keywords: difficultyBeginner, easyHack, skillScript
Depends on:
Blocks:
 
Reported: 2013-05-29 01:38 UTC by Thomas Arnhold
Modified: 2017-02-14 08:57 UTC (History)
10 users (show)

See Also:
Crash report or crash signature:


Attachments
Output of the test program (51.37 KB, text/plain)
2013-06-12 20:40 UTC, Jelle van der Waa
Details
check_headers program (1.87 KB, text/x-perl)
2013-06-13 21:52 UTC, Jelle van der Waa
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Arnhold 2013-05-29 01:38:15 UTC
With our new include directory we moved a lot of header files. All files in this directory should be included with #include <rtl/ustring.hxx> syntax.

This especially matters for the generation of precompiled headers files:

http://opengrok.libreoffice.org/xref/core/sal/inc/pch/precompiled_sal.hxx

There are a lot of duplicates in it, because of both syntax.

Here's an clean-up example: 3c18e25efdbbc13be3a0c6ed354d5e7a46feb451


This should be done fully automatically with a script, which searches for header files in include/ and replaces all local include ("rtl/ustring.hxx") statements with the global syntax (<rtl/ustring.hxx>).
Comment 1 Jelle van der Waa 2013-06-12 20:40:41 UTC
Created attachment 80742 [details]
Output of the test program
Comment 2 Jelle van der Waa 2013-06-12 20:43:16 UTC
This is the output of a test program I made to fix this issue. Atm I just need to make it search recursively through directory's. But this is the output when I run it against "configmgr/source".
Comment 3 Thomas Arnhold 2013-06-13 09:33:15 UTC
Looks promising :)
Comment 4 Jelle van der Waa 2013-06-13 21:52:41 UTC
Created attachment 80800 [details]
check_headers program

Hi, I've just fixed the header check perl script. I run it from the base of git repo. For example as:
./check_headers.pl basic
This should fix the headers, as git diff basic shows in http://sprunge.us/HNRO

What should I do with this bugreport, should I run it against the directories that need fixing and push a commit to the git repo?
Comment 5 Michael Meeks 2013-06-16 10:56:46 UTC
Looks lovely, we should get the code into solenv/bin - Jelle is pushing to gerrit I think, but we should defer running it until we've released 4.1.2 or somesuch I think to continue to make cherry-picking easy.

Thanks Jelle & welcome to LibreOffice :-)
Comment 6 Commit Notification 2013-06-17 15:34:36 UTC
Jelle van der Waa committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 clean-up headers(global/local) perl script



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 Stephan Bergmann 2013-06-20 14:51:08 UTC
See my concerns about using the <...> form for inclusion of any of our own source files in the comment I added to <https://gerrit.libreoffice.org/#/c/4306/>.  (Björn claims that using the <...> form for them would be beneficial for the performance of the Microsoft compiler, though.)
Comment 8 Björn Michaelsen 2013-06-20 21:37:16 UTC
@Stephan: No, Im not concerned about performance, but that the MSVC interpretation of "":
"This form instructs the preprocessor to look for include files in the same directory of the file that contains the #include statement, and then in the directories of any files that include (#include) that file. The preprocessor then searches along the path specified by the /I compiler option, then along paths specified by the INCLUDE environment variable."
http://msdn.microsoft.com/en-us/library/36k2cdd4%28v=vs.80%29.aspx

Might result in very hard to trace bugs and in addition differs from the gcc interpretation of:
"It searches for a file named file first in the directory containing the current file, then in the quote directories and then the same directories used for <file>. You can prepend directories to the list of quote directories with the -iquote option."
http://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html#Include-Syntax
Comment 9 Björn Michaelsen 2013-10-04 18:46:14 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 10 Thomas Arnhold 2013-11-05 08:47:49 UTC
What's actually the conses about this?
Comment 11 Michael Meeks 2013-11-05 09:25:28 UTC
The ESC discussed this, here was the minute from 2013-06-20.

I guess that it is a good time to resume work on this.

AFAIK there is/was no consensus on the list - but worth looking that out I think.

* Bulk change of header include style (Michael)
	+ thoughts on this:
	  https://bugs.freedesktop.org/show_bug.cgi?id=65108
	+ defer until 4.1.2 or so - when cherry-picking reduces
	+ concern that angle brackets only for system headers (Stephan)
	+ discussed this in the past, MS compiler does insane things
	  with path lookups if you use quotes (Bjoern)
		+ what ? (Stephan)
		+ walks entire include path backwards (Bjoern)
		+ http://msdn.microsoft.com/en-us/library/36k2cdd4%28v=vs.71%29.aspx
	+ libxml - is that a local or a system include ? (Norbert)
		+ in the code we can't know that, notion is a bit fuzzy
		+ no need to violate the std for no reason (Stephan)
			+ performance might be a good idea.
	+ potential compromise: have "" for files in the same
	  directory as the cxx (Michael)
		+ indifferent to cleaning it up (Stephan)
	+ discuss it on the list

I'd suggest my compromise: using "" for includes in the same directory as the cxx file, and <> for everything else; clearly we'd need a script to identify and re-write for those. Do we have such a thing ? :-)

Thanks for chasing !
Comment 12 Stephan Bergmann 2013-11-05 10:21:21 UTC
(In reply to comment #11)
> I'd suggest my compromise: using "" for includes in the same directory as
> the cxx file, and <> for everything else; clearly we'd need a script to
> identify and re-write for those. Do we have such a thing ? :-)

A conforming compiler is not require to support #include <...> for anything but the standard headers.  In practice, the compilers we use today do.  So, all other things being equal, the safest thing to do would be to use #include "..." for inclusion of all of LO's source files.

However, the odd behavior of MSVC described in comment 8, <http://msdn.microsoft.com/en-us/library/36k2cdd4%28v=vs.80%29.aspx> "#include Directive (C/C++)" can be used as an argument in favor of <...>.  Though I would probably use <...> exclusively then, and not even stick to "..." for inclusion of source files from the same directory.
Comment 13 Tor Lillqvist 2013-11-05 13:36:11 UTC
A conforming compiler is also allowed to support only 16-bit integers (or other similar stuff), so I don't really see the point in bringing up theoretical restrictions that no real-life compiler that our code would get near to has.
Comment 14 Stephan Bergmann 2013-11-06 07:25:29 UTC
(In reply to comment #13)
> A conforming compiler is also allowed to support only 16-bit integers

No.

> I don't really see the point in bringing up
> theoretical restrictions that no real-life compiler that our code would get
> near to has.

This is not about bringing up theoretical restrictions.  If we do such a huge cosmetic clean-up like this, and there's two ways to do it, and one is standards conforming ("...") while the other is not (<...>):  Then if there is no good reason to deviate from the standard, it only looks natural to me not to do so.
Comment 15 Michael Meeks 2013-11-06 09:34:06 UTC
Lets decide this finally in the ESC call tomorrow :-)
Comment 16 Thomas Arnhold 2013-11-06 11:17:57 UTC
And we don't need to clean up the whole code base. I have especially those headers in mind which are duplicated in the precompiled* header files due to the ".." and <..> difference.
Comment 17 Norbert Thiebaud 2013-11-06 14:56:44 UTC
I would favor that the whole source tree be cleaned-up to whatever 'norm' is agreed upon.

There are 3 proposed scheme, as I read it:

1/ use "" systematically (well except for <stdio> <assert> and other true system include I suspect....

2/ use <> systematically

3/ use "" for 'local' include, i.e in the same directory than the cxx and <> for the rest

4/ use "" for include private to the module you are in and <> for include that are 'public' header (iow system from the point of view of other module)

I favor 4/ because a/ that does solve the original pch problem b/ that convey a useful semantic content to the reader of the code: where is the include in question supposed to be.
iow in 'standard speak' considering $(SRCDIR)/include as a system include path
Comment 18 Commit Notification 2013-11-07 18:28:51 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 inter-module includes <>



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 Michael Meeks 2013-11-07 20:16:09 UTC
The ESC resolution was:

 Header includes <> vs "" etc. (Tml/Sberg)
    + easy-hack closure:
        + https://bugs.freedesktop.org/show_bug.cgi?id=65108
    + situation reasonably clear, MS compiler has a very odd use of ""
        + if we want to do a global cleanup, and be reasonably sane
          we should only use <> (Sberg)
        + problems with own vs. system version (Norbert)
            + concern with local includes vs. public ones
        + local includes have no directory path anyway (Stephan)
        + in places we do relative includes "../foo/baa.h" (Kohei)
        + helps to cleanup pre-compiled headers (Sberg)

    + we should use <> everywhere without a relative path.
        + but it's fine to continue using "" for whatever corner
          cases make sense,
          identify and restore these post-facto (Kohei)

I guess scripting this is the way to go & do it in one big commit :-)
Comment 20 Commit Notification 2013-11-09 17:49:11 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 inter-module includes <>



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 21 Commit Notification 2013-11-09 19:41:18 UTC
Norbert Thiebaud committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 inter-module includes <>



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 Commit Notification 2013-11-09 20:08:38 UTC
Norbert Thiebaud committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 inter-module includes <>



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 23 Commit Notification 2013-11-10 02:46:42 UTC
Norbert Thiebaud committed a patch related to this issue.
It has been pushed to "master":

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

fdo#65108 inter-module includes <> include/package



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 24 Julien Nabet 2014-02-10 21:39:56 UTC
Oups, I read this tracker a little too quickly.
Comment 25 Commit Notification 2015-10-12 13:02:51 UTC
Feyza Yavuz committed a patch related to this issue.
It has been pushed to "master":

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

tdf#65108 "" instead of <> written in include line

It will be available in 5.1.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 26 Commit Notification 2015-10-22 16:10:10 UTC
Feyza Yavuz committed a patch related to this issue.
It has been pushed to "master":

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

tdf#65108 use <> instead of "" in include line

It will be available in 5.1.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 27 Commit Notification 2015-11-02 11:35:06 UTC
Feyza Yavuz committed a patch related to this issue.
It has been pushed to "master":

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

tdf#65108 use <> instead of "" in include line

It will be available in 5.1.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 28 Robinson Tryon (qubit) 2015-12-14 06:53:19 UTC Comment hidden (obsolete)
Comment 29 Chris Sherlock 2016-02-14 09:22:26 UTC
I'm including a script that fixes this, and I've run it against the codebase. Gerrit submission is here:

https://gerrit.libreoffice.org/#/c/22351/
Comment 30 Robinson Tryon (qubit) 2016-02-18 14:52:07 UTC Comment hidden (obsolete)
Comment 31 jani 2016-04-18 07:25:29 UTC
A polite ping, still working on this issue?