[libcxx] r272634 - Implement variadic lock_guard.

[Eric Fiselier via cfe-commits]

Maybe we should add a new warning in Clang for this. -Wshadow diagnosis's
this but -Wshadow isn't a part of -Wall or -Wextra so it's of limited
utility.
A separate warning for shadowing 'x' caused by "T(x)" might be useful.

Do people actually use "T(x)" in the wild to default construct 'x'?

/Eric


On Wed, Jun 15, 2016 at 1:07 PM, Craig, Ben <ben.craig at codeaurora.org>
wrote:

> Makes sense.  Here's hoping parameter deduction for constructors makes it
> in!
>
> (better link)
> http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0091r2.html
>
> On 6/15/2016 1:54 PM, Eric Fiselier wrote:
>
>
> I've had a change of heart. I think that lock_guard<> has some utility in
> generic code, and I'm not sure removing it is a good idea. For example a
> function like:
>
> template <class Func, class ...Locks>
> void ExecuteUnderLocks(Func&& fn, Locks&... locks) {
>   lock_guard<Locks...> g(locks...);
>   fn();
> }
>
> I checked the proposal and it's clear that "lock_guard<>" is expected to
> compile and be default constructable. For this reason I'm not going to
> remove "lock_guard<>", at least not without further discussion.
>
>
> On Wed, Jun 15, 2016 at 12:47 PM, Craig, Ben <ben.craig at codeaurora.org>
> wrote:
>
>> On 6/15/2016 1:15 PM, Eric Fiselier wrote:
>>
>> On Wed, Jun 15, 2016 at 11:45 AM, Craig, Ben via cfe-commits <
>> <cfe-commits at lists.llvm.org>cfe-commits at lists.llvm.org> wrote:
>>
>>> Does this change (and the paper) permit declarations like the following?
>>>
>>>     lock_guard<> guard();
>>>
>>> If that syntax is allowed, then this is also likely allowed...
>>>
>>>     lock_guard<>(guard);
>>>
>>> I would really like the prior two examples to not compile.  Here is a
>>> common bug that I see in the wild...
>>>
>>>     unique_guard<mutex>(some_member_mutex);
>>>
>>> That defines a new, default constructed unique_guard named
>>> "some_member_mutex", that likely shadows the member variable
>>> some_member_mutex.  It is almost never what users want.
>>>
>>
>> I had no idea that syntax did that. I would have assumed it created an
>> unnamed temporary. I can see how that would cause bugs.
>>
>> It's also strong rationale for deduced constructor templates. (
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html)
>> auto guard = unique_guard(some_member_mutex);
>> You don't need to repeat types there, and it's very difficult to forget
>> to name the guard variable.
>>
>>
>>
>>> Is it possible to have the empty template remain undefined, and let the
>>> one element lock_guard be the base case of the recursion?  Does that help
>>> any with the mangling?
>>>
>> Nothing in the spec says the empty template should be undefined. The
>> default constructor on the empty template is technically implementing
>> "lock_guard(MutexTypes...)" for an empty pack.
>> However your example provides ample motivation to make it undefined. I'll
>> go ahead and make that change and I'll file a LWG defect to change the
>> standard.
>>
>> There is actually no recursion in the variadic lock_guard implementation,
>> so the change is trivial.
>>
>> As for mangling I'm not sure what you mean? It definitely doesn't change
>> the fact that this change is ABI breaking. (Note this change is not enabled
>> by default for that reason).
>>
>> My thought regarding the mangling was that you could still provide a one
>> argument lock_guard, as well as a variadic lock_guard.  The one argument
>> lock_guard would have the same mangling as before.  I think some of your
>> other comments have convinced me that that won't work, as I think the
>> variadic lock_guard has to be made the primary template, and I think the
>> primary template dictates the mangling.
>>
>
> Exactly.
>
>
>>
>> I'm also going to guess that throwing inline namespaces at the problem
>> won't help, as that would probably cause compile-time ambiguity.
>>
>> If I'm not mistaken, this only breaks ABI for those foolish enough to
>> pass a lock_guard reference or pointer as a parameter across a libcxx
>> version boundary.  Does that sound accurate?
>>
>
> It breaks the ABI any time "lock_guard<Mutex>" participates in the
> mangling of some function or type. In addition to your example this will
> also break any time "lock_guard<Mutex>" is used as a template parameter: ie
>
> using T = MyType<lock_guard<Mutex>>;
> MyFunction<lock_guard<Mutex>>();
>
> The two different implementations are still layout compatible, so if
> mangling were not an issue I think this change would have been safe.
>
>
>
>> --
>> Employee of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
>>
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160615/ab6e5867/attachment-0001.html>