r291905 - [Sema] Add warning for unused lambda captures

[Aaron Ballman via cfe-commits]

On Mon, Jan 23, 2017 at 5:55 PM, Nico Weber <thakis at chromium.org> wrote:
> On Mon, Jan 23, 2017 at 5:29 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, Jan 23, 2017 at 5:00 PM, Nico Weber via cfe-commits
>> <cfe-commits at lists.llvm.org> wrote:
>> > On Sun, Jan 22, 2017 at 6:17 AM, Malcolm Parsons
>> > <malcolm.parsons at gmail.com>
>> > wrote:
>> >>
>> >> On 20 January 2017 at 21:32, Nico Weber <thakis at chromium.org> wrote:
>> >> > This warns about code like
>> >> >
>> >> >   constexpr int foo = 4;
>> >> >   [&foo]() { use(foo); }
>> >> >
>> >> > That's correct, but removing &foo then makes MSVC complain about this
>> >> > code
>> >> > like "error C3493: 'foo' cannot be implicitly captured because no
>> >> > default
>> >> > capture mode has been specified". A workaround is to make foo static
>> >> > const
>> >> > instead of constexpr.
>> >> >
>> >> > This seems like an MSVC bug, but it's still a bit annoying that clang
>> >> > now
>> >> > suggests doing things that won't build in other compilers. Any ideas
>> >> > what to
>> >> > do about this?
>> >>
>> >> Should Clang care about the behaviour of other compilers that don't
>> >> follow the standard?
>> >
>> >
>> > clang should be a compiler that people like to use. Multi-platform
>> > development isn't super uncommon, so trying to give a good experience in
>> > that case seems like a good thing to me -- and we've tried to do that in
>> > the
>> > past pretty hard. It's possible that "do nothing" is the right thing,
>> > but
>> > "well that other compiler is buggy" seems like a somewhat oversimplified
>> > reply.
>>
>> We generally don't try to be bug-for-bug compatible with other
>> compilers unless the bug is sufficiently important (like it is relied
>> upon in system headers, for instance). In this case, I think the bug
>> in MSVC is annoying, but not the end of the world because it's trivial
>> to workaround in a compiler-agnostic way
>
>
> It's easy, but you need to know about it. Say you're working on some project
> that uses clang on macOS, gcc on Linux, MSVC on Windows, and you're
> developing on Linux. You write some patch, and things work fine on your
> machine. You submit your thing, it gets reverted because it triggers this
> new warning on mac. Hey, fair enough, looks like a good warning, so you
> remove the thing from the capture list and reland. Now you get reverted
> _again_ because MSVC does need that thing that clang just told you to
> revert. After three such interactions you'll be pretty unhappy, I gather :-)

I don't disagree that you'd be unhappy in that circumstance.

>> >> Cast foo to void inside the lambda.
>> >
>> >
>> > That's equivalent to disabling the clang warning in those cases (which
>> > would
>> > be a shame, since the warning is super useful, and it doesn't introduce
>> > problems in most cases -- only with local constexprs). Maybe the warning
>> > could be split in two, so that it can be toggled separately for
>> > constexpr?
>>
>> While functional, that seems like a lot of work for this case.
>
>
> It's like 4 lines, no?

Yes, but no. It's 4 lines that introduce a compiler flag that we have
to support forever, even if Microsoft eventually fixes their bug
(making the purpose to the compiler flag entirely moot), which is what
my concern boils down to. If this was a common problem, then I would
think the flag (or some other, heavier fix) might actually be worth
having to keep around forever. Since it doesn't really seem to be a
common issue, I'm not keen on adding a user-facing compiler flag
because of the maintenance burden.

~Aaron

> As I said, I'm not sure it's worth it either. As-is, I think larger cross
> platform teams must enable this warning for the reason mentioned above.
> Requiring this is a bit of a shame since I think it's a great warning, so I
> was hoping we could come up with a way so we could use the warning as well.
>
>>
>> However, the downside to the cast to void is that when MSVC does fix
>> the bug, the diagnostic remains silenced in the user's source base.
>>
>> >
>> >>
>> >> Only capture foo when building with MSVC.
>> >>
>> >> Stop building with MSVC.
>> >
>> >
>> > This reply like this make me believe you think it was silly to even
>> > report
>> > this problem. I don't think it's silly, and there's ample precedent in
>> > clang
>> > to make it work well in the actual world instead of in a world we wished
>> > we
>> > lived in.
>>
>> Definitely not silly to report the problem, but at the same time, that
>> precedent is usually reserved for things that affect a significant
>> portion of users when the issue boils down to a compiler bug in
>> another compiler. How common of a problem is this use case? Does it
>> crop up in important header files?
>
>
> It's not super common. Ivan, I think you ran into this twice in all of
> Chromium?
>
>>
>>
>> ~Aaron
>>
>> >
>> >>
>> >> Complain to Microsoft.
>> >
>> >
>> > Already done. The reality is that their shipping compiler produces this
>> > diagnostic, and the current 2017 build does too.
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >
>
>