[PATCH] Move optional into the experimental namespace (first bit of the TSes)

Marshall Clow mclow.lists at gmail.com
Thu Nov 14 11:03:16 PST 2013


On Nov 12, 2013, at 3:56 PM, Marshall Clow <mclow.lists at gmail.com> wrote:

> On Nov 12, 2013, at 3:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 
>> I assume the intent is to implement this paper:
>> 
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3793.html
>> 
>> So, assuming that include/experimental/optional was copied from include/optional (the diff doesn't say where the 'old' lines come from here)…
> 
> Yes.
> 
>> You included the inline namespace in the library synopsis comment; it shouldn't be there.
> 
> Ok.

Fixed.

> 
>> bad_optional_access should be in namespace std::experimental, not in namespace std.
> 
> I went back and forth about that. I agree - it _should_ be there.
> 
> However:
> 	The destructor for bad_optional_access lives in libc++.dylib. (due to a peculiarity in the Itanium ABI).
> 	If I put bad_optional_access in namespace std, then users do not need a new dylib when optional moves from std::experimental to std.

I put the bad_optional_access into "std::experimental" (but not into "std::experimental::__library_fundamentals_v1")

> 
>> For headers in include/support, the source files are in src/support. Should optional.cpp be moved to src/experimental/optional.cpp?
> 
> Good question. 
> Header folder structure is an interface.
> Source folder structure is not.
> 
> I don't see that this is necessary; but if other people really want it, I have no objection to changing it.

I did not change this.

>> Bikeshedding on the name __TSLibraryFundamentals:
>>  * Should this contain a version number? We may want an ABI break at some point, and it might be nicer to have always had the version number in the name.
> 
> Yes! Crap - there was supposed to be a _v1 on the end of that.
> 

>>  * Other library-internal names in libc++ seem to use __lower_case_names
>> Maybe __library_fundamentals_1?
> 
> That's fine - I can live with either one.
> 

Changed to __library_fundamentals_v1.

I also cleaned up a bunch of the tests with "using" directives.
New patch attached.
Thanks for the review.

-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
        -- Yu Suzuki
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131114/980ac48b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: optional-ts2.patch
Type: application/octet-stream
Size: 111324 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131114/980ac48b/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131114/980ac48b/attachment-0001.html>


More information about the cfe-commits mailing list