[PATCH] Move optional into the experimental namespace (first bit of the TSes)
Howard Hinnant
howard.hinnant at gmail.com
Fri Nov 15 14:43:45 PST 2013
Thanks, please commit.
Howard
On Nov 14, 2013, at 2:03 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>
> 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
> <optional-ts2.patch>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list