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>).
Created attachment 80742 [details] Output of the test program
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".
Looks promising :)
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?
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 :-)
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.
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.)
@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
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
What's actually the conses about this?
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 !
(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.
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.
(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.
Lets decide this finally in the ESC call tomorrow :-)
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.
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
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.
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 :-)
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.
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.
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.
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.
Oups, I read this tracker a little too quickly.
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.
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.
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.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillScript ) [NinjaEdit]
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/
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
A polite ping, still working on this issue?