Bug Hunting Session
Bug 95416 - Get rid of #include "../foo/bar.hxx" style includes
Summary: Get rid of #include "../foo/bar.hxx" style includes
Status: RESOLVED NOTABUG
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.3.0 target:5.4.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2015-10-29 15:07 UTC by Tor Lillqvist
Modified: 2017-03-15 12:10 UTC (History)
10 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 Tor Lillqvist 2015-10-29 15:07:00 UTC
We have a couple of hundred cases where a source file includes an include file like this:

#include "../foo/bar.hxx"
or
#include <../foo/bar.hxx>

That is generally considered ugly, I think. In most cases, the file being included should be moved to an appropriate "inc" directory in the module in question, and then be included with a simple:

#include "bar.xx"

because if an include file is used by source files in multiple directories, clearly it isn't strictly local and should be in some "inc" directory.

One special case is the gtk2/gtk3 mess in vcl, which probably should be left as is.
Comment 1 Commit Notification 2015-10-29 15:21:07 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416: Get rid of #include "../foo/bar.hxx" style includes

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 2 Tor Lillqvist 2015-10-29 15:22:16 UTC
(The above is meant to be an example, done for just one module.) (And when we say "module", we mean subdirectory, nothing fancier.)
Comment 3 Buovjaga 2015-11-09 09:28:08 UTC
Status -> NEW.
Comment 4 Robinson Tryon (qubit) 2015-12-13 11:04:05 UTC Comment hidden (obsolete)
Comment 5 Commit Notification 2015-12-24 09:33:34 UTC
burcinakalin committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include "../foo/bar.hxx" style includes

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 6 Commit Notification 2016-01-27 21:20:01 UTC
burcinakalin committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include "../foo/bar.hxx" style includes

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 7 Commit Notification 2016-02-03 09:05:17 UTC
burcinakalin committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include "../foo/bar.hxx" style includes.

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 8 Robinson Tryon (qubit) 2016-02-18 14:52:25 UTC Comment hidden (obsolete)
Comment 9 Commit Notification 2016-05-01 10:20:42 UTC
Burcin Akalin committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include "../foo/bar.hxx" style includes

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 10 jani 2016-06-14 10:01:15 UTC
Controlled, still open
Comment 11 jani 2016-08-05 19:02:04 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
Comment 12 Commit Notification 2016-08-23 05:42:51 UTC
Jochen Nitschke committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 remove ../ style include

It will be available in 5.3.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 13 Commit Notification 2016-08-26 13:15:54 UTC
Jochen Nitschke committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 remove ../ style include

It will be available in 5.3.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 14 Commit Notification 2016-10-16 12:44:05 UTC
Dilek Uzulmez committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include ../foo/bar.hxx style includes

It will be available in 5.3.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 15 Commit Notification 2016-10-17 08:02:55 UTC
Arnold Dumas committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416: Get rid of #include ../foo/bar.hxx style includes

It will be available in 5.3.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 16 adamkasztenny 2016-11-03 00:05:57 UTC
There are still 218 occurrences of this, by my count, using git grep -E '#include (<|")\.\./'

I'll take a look at this and maybe write a script that replaces all of these occurrences (except the gtk* ones mentioned)
Comment 17 Tor Lillqvist 2016-11-03 06:35:40 UTC
By all means do write a script to help you, but don't blindly just run a script on all instances and assume the result compiles on all platforms. Do it in pieces, and wait for confirmation from Jenkins that it actually compiles on all platforms.
Comment 18 adamkasztenny 2016-11-03 12:26:46 UTC
(In reply to Tor Lillqvist from comment #17)
> By all means do write a script to help you, but don't blindly just run a
> script on all instances and assume the result compiles on all platforms. Do
> it in pieces, and wait for confirmation from Jenkins that it actually
> compiles on all platforms.

Yes of course.
Comment 19 Commit Notification 2016-12-12 08:55:08 UTC
Adam Kasztenny committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix an include for xmlsecurity

It will be available in 5.4.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 Commit Notification 2016-12-13 12:01:29 UTC
Adam Kasztenny committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix includes for xmloff

It will be available in 5.4.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 21 Commit Notification 2016-12-13 12:04:01 UTC
Adam Kasztenny committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix an include for sfx2

It will be available in 5.4.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 Commit Notification 2016-12-20 16:59:55 UTC
Huzaifa Iftikhar committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix an include for desktop directory

It will be available in 5.4.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 23 Commit Notification 2016-12-21 09:02:52 UTC
Huzaifa Iftikhar committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix an include for 2 files in unopkg

It will be available in 5.4.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 24 Commit Notification 2016-12-25 07:36:12 UTC
tamsil1amani3 committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Fix include file

It will be available in 5.4.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 25 Commit Notification 2017-01-02 19:50:19 UTC
Gaurav Dhingra committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Get rid of #include ../foo/bar.hxx style includes

It will be available in 5.4.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 2017-01-03 14:51:58 UTC
abdulwd committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Two include files moved to sc/inc

It will be available in 5.4.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 Jeevan Surya 2017-03-02 17:39:21 UTC
other than the case of gtk2/gtk3 in vcl, I have checked that such cases still exists in nearly 4 - 5 other folders, so, can I assign this task to myself. I guess I will be able to do it, without any errors.

What I understood is, we just need to make a local copy of the required include file to the folder, which actually uses it ( by ../ ).
Comment 28 Tor Lillqvist 2017-03-02 17:53:00 UTC
No *copying* of files should be done. Just look at one of the examples that you see of what has already been done here.
Comment 29 Jeevan Surya 2017-03-02 18:16:57 UTC
so, do we need to move the include file, from ../../  to the inc folder of the parent directory.
Comment 30 Commit Notification 2017-03-07 02:51:31 UTC
Jeevan committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 Removed ../ in filter/source/svg/test files

It will be available in 5.4.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 31 Commit Notification 2017-03-13 18:55:29 UTC
Jeevan committed a patch related to this issue.
It has been pushed to "master":

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

tdf#95416 remove ../ in #include headers of sfx2 files

It will be available in 5.4.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 32 Eike Rathke 2017-03-15 00:47:49 UTC
ARGH! Can we please be VERY SENSIBLE about this? It leads to blindly moving headers into a module's global inc/ directory, making them available to each and every source file of that module effectively destroying the separation of view,core,filter even further. Examples of such bad moves are https://cgit.freedesktop.org/libreoffice/core/commit/?id=a0f6374670f67646e802aef45d927a8746b2ae12 moving Calc ViewData around and the just pending https://gerrit.libreoffice.org/34827 that even wanted to move ScDocShell which would be an invitation to use it about ~everywhere.

I rather prefer having some sporadic
#include "../../ui/inc/xxx.hxx"
if really necessary than letting run this loose. Specifically UI view related classes are not to be offered for general use even if now and then they may be needed by filter code.

I also consider deciding how to solve things by no means an EasyHack in all cases unless we don't care about model separation anymore.

Or am I just on the wrong track and everything is fine?
Comment 33 Tor Lillqvist 2017-03-15 06:41:25 UTC
But isn't it *using* stuff from a wrong include file that indicates something is wrong, not where the include file is located? Anyway, I'm fine with resolving this as NOTABUG; Eike is right that no easy hacker will understand the subtleties involved. Not even I do.

Should we instead then have a difficultyInteresting easyHack (a contradiction in terms...) to find these includes and fix the code to not need stuff from such a "wrong" include file at all?
Comment 34 Michael Meeks 2017-03-15 08:18:52 UTC
Jeevan - so sorry you hit something that turned out to be controversial ! =) at least finding that out was helpful for the project; so good work.

Any my 2 cents on headers; personally I -really- like headers to be well scoped - Calc is quite good here, but even so -so- many modules like to use ../inc/ for headers that would be IMHO -much- better placed right next to the source file they refer to, and which have no usage outside that scope which I find rather irritating personally ;-) But - each to their own ... =)
Comment 35 Eike Rathke 2017-03-15 12:10:23 UTC
(In reply to Tor Lillqvist from comment #33)
> But isn't it *using* stuff from a wrong include file that indicates
> something is wrong, not where the include file is located?
Yes it is, though maybe inevitable in some filter code cases, but moving the file to a higher hierarchy level and adapting the include effectively hides that and invites for further abuse all over the place.

> Should we instead then have a difficultyInteresting easyHack (a
> contradiction in terms...) to find these includes and fix the code to not
> need stuff from such a "wrong" include file at all?

We can try.. but having taken a look at the mentioned ScDocShell case it's definitely not just a five minute task [1] ... and no quick idea how it actually could be solved. difficultyInteresting certainly is applicable ;-)

However, there *are* the easy cases where moving a header is appropriate, but it has to be decided on a case by case basis keeping header scope in mind, and reviewers should be aware of the underlying problem.

[1] http://hea-www.harvard.edu/~fine/opinions/fiveminute.html