Bug Hunting Session
Bug 86929 - Kill FOREACHPAM_START for good
Summary: Kill FOREACHPAM_START for good
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: low trivial
Assignee: Julien Nabet
URL:
Whiteboard: target:4.5.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-12-02 10:40 UTC by Björn Michaelsen
Modified: 2016-02-18 16:37 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 Björn Michaelsen 2014-12-02 10:40:16 UTC
FOREACHPAM_START/FOREACHPAM_END were horrible macros that iterated over a Ring. With the new sw::Ring class in sw/inc/ring.hxx having proper iterators, they are obsolete and can be replaced with standard BOOST_FOREACH (and hopefully one day directly with C++11s for( : ) iteration.

For now, lets kill FOREACHPAM_START/FOREACHPAM_END like done in e.g.:
 http://cgit.freedesktop.org/libreoffice/core/commit/?id=2b433d87525918bf8d51fe164ffea9c9099b52ce

While doing that, please look out for opportunities to use more constness in the iteration: The old macros carelessly did cast away constness, so check if there is room for improvement there.

Bonus points for walking through general calls to sw::Ring<>s GetPrev()/GetNext() which are now stronger typed (returning a SwPaM* or SwViewShell*) and check if there now is superfluous casting there that can be removed.
Comment 1 Björn Michaelsen 2014-12-02 10:53:07 UTC
adding LibreOffice developer list as CC to unresolved Writer EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 2 Miklos Vajna 2014-12-02 10:57:21 UTC
FWIW, C++11 range-based for-loops are OK on libreoffice-4-4 and master, best to not use BOOST_FOREACH in new code, I guess.
Comment 3 Björn Michaelsen 2014-12-02 12:17:37 UTC
We cant for now have sw::Ring<> implement STL-like begin()/end() functions, see:
 https://gerrit.libreoffice.org/gitweb?p=core.git;a=blob;f=sw/inc/ring.hxx;h=d58733f1459fda302a99d1ea9d56da82c55f9ca9;hb=2dd7cc5b925d0b4c62553eeba9f6524ce7b6217b#l105

So for now, BOOST_FOREACH should still be an improvement over the old const-eating homegrown macros. Once none of the derived classes of sw::Ring<> are deriving from any other STL-container, we should be able to trivially replace BOOST_FOREACH with a proper C++11 style for() iteration.
Comment 4 Commit Notification 2014-12-03 08:25:24 UTC
Bjoern Michaelsen committed a patch related to this issue.
It has been pushed to "master":

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

fdo#86929: Kill FOREACHPAM_START for good

It will be available in 4.5.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 5 Björn Michaelsen 2014-12-03 08:28:30 UTC
With some additional thinking, I found a way to get C++11 for(:) iteration right away, without using the BOOST_FOREACH hack as an intermediate solution. See:
 http://cgit.freedesktop.org/libreoffice/core/commit/?id=02ef3abd83f2d5f702349a8cd47928621ee5c620
for an example change. Thus also not depending on fdo#75757 anymore. This should make this Easy Hack even simpler.
Comment 6 Julien Nabet 2014-12-13 18:36:50 UTC
I submitted for review these:
- https://gerrit.libreoffice.org/#/c/13465/
- https://gerrit.libreoffice.org/#/c/13466/
but I don't know how to do for those in sw/source/core/doc/, see http://opengrok.libreoffice.org/search?q=FOREACHPAM_START&project=core&defs=&refs=&path=&hist=
Comment 7 Commit Notification 2014-12-15 21:05:54 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related fdo#86929: Kill FOREACHPAM_START for good (part2)

It will be available in 4.5.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 Commit Notification 2014-12-15 21:07:12 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related fdo#86929: Kill FOREACHPAM_START for good (part1)

It will be available in 4.5.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 9 Commit Notification 2014-12-18 06:41:32 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

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

Related fdo#86929: Kill FOREACHPAM_START for good (final part)

It will be available in 4.5.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 Robinson Tryon (qubit) 2015-12-16 00:04:29 UTC
Migrating Whiteboard tags to Keywords: (EasyHack SkillCpp DifficultyBeginner TopicCleanup )
[NinjaEdit]
Comment 11 Robinson Tryon (qubit) 2016-02-18 16:37:12 UTC
Remove LibreOffice Dev List from CC on EasyHacks
(curtailing excessive email to list)
[NinjaEdit]