boostorg / ptr_container

Boost.org ptr_container module
http://boost.org/libs/ptr_container
Boost Software License 1.0
13 stars 41 forks source link

Documentation updates for PR #8 #16

Closed cstratopoulos closed 6 years ago

cstratopoulos commented 6 years ago

This issue is in reference to https://github.com/boostorg/ptr_container/pull/8 which conditionally provides interfaces with auto_ptr or unique_ptr to work around auto_ptr deprecation with newer compilers.

If I understand correctly, this PR was too late for Boost 1.66 having been merged in late November, but it will be in Boost 1.67; moreover there is still time to submit documentation updates before the 1.67 release.

I noticed on the 1.67 beta snapshot documentation that auto_ptr is still uniformly referred to in documentation, with no mention of unique_ptr.

I'm opening this issue just to bring attention to this matter, but also maybe it's appropriate to have a discussion on how to treat this situation. One thought I had was that in other projects it's common practice to use a special identifier like implementation-defined in lieu of actual object types, with a documentation page explaining its meaning. Maybe auto_ptr<T> could be replaced with something like smart-pointer-type<T>, with a documentation page explaining that Boost.Config will be used to resolve this to either auto_ptr<T> or unique_ptr<T> depending on compiler standard? Maybe it would be appropriate to add this to an "Upgrading from Boost v. 1.66.*" heading, along the lines of similarly named headings on the documentation homepage.

eldiener commented 6 years ago

I appreciate your bringing this up. The doc is written in reStructuredText. I am not the original maintainer or else I would have written the doc using Boost quickbook. If you would care to create a PR with changes to the appropriate .rst files I would be glad to take a look at it and merge it in order to produce updated docs for the library.

cstratopoulos commented 6 years ago

@eldiener Understood, and thanks for getting back to me. Will try to get started on something on my lunch break or after work today, hopefully will have a PR ready sometime tomorrow.

edit: Just to confirm I follow correctly--my understanding is that documentation is maintained in the reStructuredText files, and a shell script is used to invoke rst to html conversion on all of them. So I expect I should only edit the rst files directly, but in my PR I should also commit the html files generated from invoking the script?

eldiener commented 6 years ago

Take your time with the PR, there is no rush. You are correct about also committing the HTML files generated by the rst file changes.

cstratopoulos commented 6 years ago

@eldiener I've had some time to get started on this, it turned out to be a bit more complicated than expected, mostly due to the fact that reStructuredText does not support nested markup.

I wanted to send you some of the work I've started for feedback before proceeding further. Essentially the approach I have in mind follows, e.g., Boost.Asio documentation where a keyword such as void-or-deduced is always presented as an italicized, monospaced hyperlink to an explanatory page. (example)

I have created the following branch on my fork where I'm working on the updated documentation: https://github.com/cstratopoulos/ptr_container/tree/feature/auto_ptr-deprecate-doc-update/doc

Main structural features I would like to point out:

Also I am of course open to another name besides compatible-smart-ptr, an obvious alternative I thought of was auto-or-unique-ptr or something to that effect.

Given the limitations of rst, some searching online suggested that most users just work around this by editing raw HTML. The most straightforward way I've thought of to do this is shown here: https://github.com/cstratopoulos/ptr_container/blob/feature/auto_ptr-deprecate-doc-update/doc/comp_rever_ptr_container.sh

If anyone called this a distasteful addition to the previously-spartan shell script I would agree; I am open to other suggestions.

As a result of handling italic hyperlinking in the shell script, you may have to save a local copy of this file to see the effect: https://github.com/cstratopoulos/ptr_container/blob/feature/auto_ptr-deprecate-doc-update/doc/reversible_ptr_container.html

eldiener commented 6 years ago

As long as you can maintain the possible creation of the html pages from the shell scripts I would not worry about how you do it. Your upgrade section is excellent and the name of 'compatible-smart-ptr' is fine and better than 'auto-or-unique-ptr'. But are you sure that the pointer container interface replaces auto_ptr with unique_ptr when both are available as opposed to adding unique_ptr as an alternative when both are available and replacing auto_ptr with unique_ptr only when auto_ptr is no longer available. I did not code those changes.

cstratopoulos commented 6 years ago

@eldiener That is a good catch--I just took a closer look at the way the changes are implemented, and the wording for BOOST_NO_AUTO_PTR and BOOST_NO_CXX11_SMART_PTR.

In the case of auto_ptr parameters it is exactly as you said:

adding unique_ptr as an alternative when both are available and replacing auto_ptr with unique_ptr only when auto_ptr is no longer available.

And that can be seen here for example.

When it comes to function return types such overloading is of course not possible, so we get a unique_ptr return precisely when auto_ptr is not available, as seen e.g., here

Anyway I've finished making all the changes to individual files. Probably tomorrow I'll correct the upgrading section and update the compatible smart pointer page to reflect the parameter vs. return type subtlety, then go ahead an open the PR. Thanks again for your feedback thus far.