Boost C++ Libraries: Ticket #2056: Please add support for built-in arrays to boost::swap

attachment set

niels_dekker — Sun, 29 Jun 2008 17:36:40 GMT

attachment → array_support_for_sandbox_swap.patch

attachment set

niels_dekker — Sun, 29 Jun 2008 18:18:59 GMT

attachment → swap_arrays.cpp

swap_arrays.cpp test file, to be added to libs/utility/swap/test

owner set

Dave Abrahams — Tue, 01 Jul 2008 16:51:09 GMT

owner set to niels_dekker

I tried to move the sandbox and apply these patches, but the sandbox is at least missing something to link the documentation into the rest of the Boost docs, a Jamfile for testing, and an editorial pass over the docs (at least one glaringly incomplete sentence). I really want to get this into Boost but don't have the time to fix these minor problems up.

Also, it would help if you would supply a *single* unified patch to the trunk that adds this stuff. Running patch multiple times and locating the files to be patched is pretty tedious :-)

Thanks

cc set

Dave Abrahams — Tue, 01 Jul 2008 18:29:14 GMT

cc joseph.gauterin@… added

attachment set

niels_dekker — Tue, 01 Jul 2008 22:13:56 GMT

attachment → array_support_for_sandbox_swap_html.patch

Documentation update regarding array support, to be applied to libs/utility/swap.html Version 2, including a note by Joseph Gauterin

Dave Abrahams — Wed, 02 Jul 2008 01:19:19 GMT

Again, please submit a single patch that covers all the files added or modified.

I see that Jamfile.v2 is there, but like the docs need to be linked into the rest of the Boost documentation it needs to be linked into the top level testing Jamfile.

attachment set

niels_dekker — Wed, 02 Jul 2008 07:44:18 GMT

attachment → single_patch_of_boost_array_swap_modifications.patch

Single patch that covers all the files added or modified, including Jamfile.v2

niels_dekker — Wed, 02 Jul 2008 08:08:19 GMT

Hereby submitted a single patch that covers all array-swap related changes to https://svn.boost.org/svn/boost/sandbox/swap

I wouldn't mind doing the commit to the sandbox myself, now that Joseph has also taken a look. (I do have SVN write access.)

Also I wouldn't mind adding a link, href="swap.html", to /utility/index.html once you've moved boost::swap the trunk. (But I guess that's beyond the scope of this ticket.)

I guess the contents of sandbox/swap/libs/utility/swap/test/Jamfile.v2 need to be copied into trunk/libs/utility/test/Jamfile.v2 right? Or would you prefer to have "swap" as a separate test suit?

cc changed

niels_dekker — Wed, 02 Jul 2008 11:24:01 GMT

cc niels_dekker added

Dave Abrahams — Wed, 02 Jul 2008 13:03:24 GMT

Replying to niels_dekker:

Hereby submitted a single patch that covers all array-swap related changes to https://svn.boost.org/svn/boost/sandbox/swap

I wouldn't mind doing the commit to the sandbox myself, now that Joseph has also taken a look. (I do have SVN write access.)

Also I wouldn't mind adding a link, href="swap.html", to /utility/index.html once you've moved boost::swap the trunk. (But I guess that's beyond the scope of this ticket.)

No, that's exactly what I meant by "something to link the documentation into the rest of the Boost docs"

I guess the contents of sandbox/swap/libs/utility/swap/test/Jamfile.v2 need to be copied into trunk/libs/utility/test/Jamfile.v2 right? Or would you prefer to have "swap" as a separate test suit?

Either way is OK. If you can anticipate "swap" needing a few more tests, I'd vote for the separate suite, otherwise, just integrate it into the existing Jamfile. But what I'm asking for is a patch against the trunk, so your patch would include the modifications to that Jamfile.

If you have commit access on the trunk, well then, please go ahead and commit it yourself.

niels_dekker — Wed, 02 Jul 2008 13:59:39 GMT

Replying to dave:

But what I'm asking for is a patch against the trunk, so your patch would include the modifications to that Jamfile.

Sorry, I still don't get it. How can I create an array-swap patch for the trunk if "swap" itself isn't yet in the trunk?

Isn't it easier to just apply the patch to the sandbox, beforehand? So shall I apply the patch to the sandbox?

If you can anticipate "swap" needing a few more tests, I'd vote for the separate suite,

Okay, let's have it as separate suite! Especially because I would like to have new boost::swap_ranges and boost::iter_swap functions added to a future version of boost/swap, which would need extra tests, of course...

niels_dekker — Wed, 02 Jul 2008 21:01:36 GMT

So I just applied the array-swap patch to the sandbox (changeset [47003]), and fixed some small typo's ([47004]). Please double check.

Is there anything more to be done, before moving sandbox/swap to the trunk? An extra editorial pass over the docs?

FYI, sandbox/swap consists of:

  boost/swap.hpp
  boost/utility/swap.hpp
  libs/utility/swap.html
  libs/utility/swap/test/Jamfile.v2
  libs/utility/swap/test/*.cpp (13 files)

anonymous — Wed, 02 Jul 2008 23:05:07 GMT

Replying to niels_dekker:

Sorry, I still don't get it. How can I create an array-swap patch for the trunk if "swap" itself isn't yet in the trunk?

Simple:

Check out a clean copy of the trunk.
Integrate the stuff you put in the sandbox into your local copy of the trunk.
cd $BOOST_ROOT in your trunk copy
svn diff

The result is your patch

Dave Abrahams — Wed, 02 Jul 2008 23:10:21 GMT

Replying to niels_dekker:

Is there anything more to be done, before moving sandbox/swap to the trunk?

Please prepare the patch against trunk as described above. That includes making modifications to status/Jamfile.v2 (or whatever) that integrates the suite, and modifications to the existing webpages that link to the swap docs.

An extra editorial pass over the docs?

Well, it still says "is available via argument dependent" with no "lookup," so it probably wouldn't hurt :-)

attachment set

niels_dekker — Thu, 03 Jul 2008 18:35:08 GMT

attachment → status_and_utility_index.patch

Patch, adding "swap" to status/Jamfile.v2 and libs/utility/index.html

niels_dekker — Thu, 03 Jul 2008 18:37:56 GMT

Replying to dave:

Please prepare the patch against trunk as described above. That includes making modifications to status/Jamfile.v2 (or whatever) that integrates the suite, and modifications to the existing webpages that link to the swap docs.

I got the impression that a SVN merge cannot be part of a patch. (But I could be wrong. Am I?) Anyway, the modifications to status/Jamfile.v2 and libs/utility/index.html are included with the attached patch, status_and_utility_index.patch Please double check.

Well, it still says "is available via argument dependent" with no "lookup," so it probably wouldn't hurt :-)

Fixed: [47032]

Note: Joseph Gauterin just mailed me that he would like to do the move to the trunk. Which is perfectly fine by me, because it's his boost::swap in the first place.

attachment set

joseph.gauterin — Fri, 04 Jul 2008 00:00:27 GMT

attachment → swap_to_trunk.patch

joseph.gauterin — Fri, 04 Jul 2008 00:08:31 GMT

I've attached a patch to be applied to the trunk.

As well as the moving the contents of sandbox/swap to the correct places on the trunk, it alters the follwoing files: /libs/libraries - adds sqap to the list of libraries /libs/maintainers - added my details as maintainer of boost::swap /libs/utility/index.html - added link to swap documentation /status/Jamfile.v2 - integrated utility/swap test suite

Dave - I don't have SVN write access to the trunk, so will you please apply the patch.

Thanks, Joe.

owner changed

Dave Abrahams — Fri, 04 Jul 2008 23:44:53 GMT

owner changed from niels_dekker to John Maddock

I'm sorry, but the patch doesn't apply cleanly. It's partly due to [43633#file2], which appears to be an at-least-partly-bogus change (John?) and partly because it chokes on the changes to status/Jamfile.v2 for reasons I don't understand:

patching file Jamfile.v2
Hunk #1 FAILED at 112.
Hunk #2 FAILED at 126.
2 out of 2 hunks FAILED -- saving rejects to file Jamfile.v2.rej

Dave Abrahams — Fri, 04 Jul 2008 23:47:27 GMT

Hmm, maybe this isn't John's problem. I don't know why iterator adaptors are referenced by the utility/ docs. However, I also don't understand why he added generator_iterator in this change

owner changed

Dave Abrahams — Sat, 05 Jul 2008 01:38:58 GMT

owner changed from John Maddock to joseph.gauterin

OK, sorry, my mistake. I needed -p0 in my patch command, and I was able to apply it just fine. However, the svn pre-commit hook failed. Since Joseph is going to be the maintainer of this stuff (very nice, thorough tests, Joseph!) I just gave him commit privileges on trunk. Joseph, the ball is in your court.

John Maddock — Sat, 05 Jul 2008 08:16:30 GMT

Replying to dave:

Hmm, maybe this isn't John's problem. I don't know why iterator adaptors are referenced by the utility/ docs. However, I also don't understand why he added generator_iterator in this change

I believe the generator_iterator.htm was an unreferenced file showing up in the inspection report - I added a link to it that's all.

John.

status changed; resolution set

joseph.gauterin — Sat, 05 Jul 2008 11:33:47 GMT

status new → closed
resolution → fixed

I've committed the change now.

Joe.

anonymous — Mon, 18 Aug 2008 13:45:18 GMT

And why this library was moved to the trunk without any formal review?

Dave Abrahams — Mon, 18 Aug 2008 20:03:13 GMT

It is going to be a part of the existing "utility" library.