<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>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 <a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D18271">http://reviews.llvm.org/D18271</a><br>
</p>
<br>
<div class="moz-cite-prefix">On 6/15/2016 2:22 PM, Eric Fiselier
wrote:<br>
</div>
<blockquote
cite="mid:CAB=TDAWwTh4qz9fR__HnESwGMzSx5Nqgj+Rk226JTQVvskk8Kg@mail.gmail.com"
type="cite">
<div dir="ltr">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.
<div>A separate warning for shadowing 'x' caused by "T(x)" might
be useful.</div>
<div><br>
</div>
<div>
<div>
<div>Do people actually use "T(x)" in the wild to default
construct 'x'?<br>
<div><br>
</div>
<div>/Eric<br>
<div><br>
</div>
</div>
</div>
</div>
</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Jun 15, 2016 at 1:07 PM, Craig,
Ben <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:ben.craig@codeaurora.org" target="_blank">ben.craig@codeaurora.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<p>Makes sense. Here's hoping parameter deduction for
constructors makes it in!</p>
<p>(better link) <a moz-do-not-send="true"
href="http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0091r2.html"
target="_blank">http://open-std.org/JTC1/SC22/WG21/docs/papers/2016/p0091r2.html</a><br>
</p>
<div>
<div class="h5"> <br>
<div>On 6/15/2016 1:54 PM, Eric Fiselier wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div>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:
<div><br>
</div>
<div>template <class Func, class ...Locks></div>
<div>void ExecuteUnderLocks(Func&& fn,
Locks&... locks) {</div>
<div> lock_guard<Locks...> g(locks...);</div>
<div> fn();</div>
<div>}</div>
<div><br>
</div>
<div>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.</div>
<div> </div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Wed, Jun 15, 2016 at
12:47 PM, Craig, Ben <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:ben.craig@codeaurora.org"
target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:ben.craig@codeaurora.org">ben.craig@codeaurora.org</a></a>></span>
wrote:<br>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><span>
On 6/15/2016 1:15 PM, Eric Fiselier
wrote:<br>
<blockquote type="cite">
<div dir="ltr">On Wed, Jun 15, 2016 at
11:45 AM, Craig, Ben via cfe-commits
<span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:cfe-commits@lists.llvm.org"
target="_blank"><a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a></a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">
<div bgcolor="#FFFFFF"
text="#000000">
<p>Does this change (and the
paper) permit declarations
like the following?</p>
<p> lock_guard<>
guard();</p>
<p>If that syntax is
allowed, then this is also
likely allowed...</p>
<p>
lock_guard<>(guard);</p>
<p>I would really like the
prior two examples to not
compile. Here is a common
bug that I see in the
wild...</p>
<p>
unique_guard<mutex>(some_member_mutex);</p>
<p>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.<br>
</p>
</div>
</blockquote>
<div><br>
</div>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
</span> It's also strong rationale for
deduced constructor templates. (<a
moz-do-not-send="true"
href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html"
target="_blank"><a class="moz-txt-link-freetext" href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html">http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0091r0.html</a></a>)<br>
auto guard =
unique_guard(some_member_mutex); <br>
You don't need to repeat types there, and
it's very difficult to forget to name the
guard variable.<span><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote"
style="margin:0 0 0
.8ex;border-left:1px #ccc
solid;padding-left:1ex">
<div bgcolor="#FFFFFF"
text="#000000">
<p> </p>
<p>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?<br>
</p>
</div>
</blockquote>
<div>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.</div>
<div>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.</div>
<div><br>
</div>
<div>There is actually no
recursion in the variadic
lock_guard implementation, so
the change is trivial.</div>
<div><br>
</div>
<div>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).</div>
</div>
</div>
</div>
</blockquote>
</span> 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.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Exactly.</div>
<div> </div>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
I'm also going to guess that throwing
inline namespaces at the problem won't
help, as that would probably cause
compile-time ambiguity.<br>
<br>
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?</div>
</blockquote>
<div><br>
</div>
<div>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</div>
<div><br>
</div>
<div>using T =
MyType<lock_guard<Mutex>>;</div>
<div>MyFunction<lock_guard<Mutex>>();</div>
<div><br>
</div>
<div>The two different implementations are
still layout compatible, so if mangling were
not an issue I think this change would have
been safe.</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote"
style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <span>
<pre cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
</pre>
</span></div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<pre cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
</pre>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
</pre>
</body>
</html>