[libcxx] r272634 - Implement variadic lock_guard.

[Eric Fiselier via cfe-commits]

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> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160615/5c40b943/attachment-0001.html>