[libcxx] r272634 - Implement variadic lock_guard.
Craig, Ben via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 15 12:32:15 PDT 2016
It would be awesome if this kind of shadowing warning could be put into
-Wall. My recollection on the last set of -Wshadow reviews is that most
shadowing warnings are from ctor arguments being used to initialize
members. Here's the last discussion / review regarding shadowing
http://reviews.llvm.org/D18271
On 6/15/2016 2:22 PM, Eric Fiselier wrote:
> 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
> <mailto: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 <mailto: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
>>> <mailto: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
>
>
--
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/0726f9ed/attachment-0001.html>
More information about the cfe-commits
mailing list