<div dir="ltr">Awesome. Thank you all! :)<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 1, 2017 at 9:25 AM, Hans Wennborg <span dir="ltr"><<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Merged this (r291963) in r293783.<br>
<br>
And the others (r293360 + r293369) in r293784.<br>
<br>
Thanks,<br>
Hans<br>
<div class="m_-469846290612175493HOEnZb"><div class="m_-469846290612175493h5"><br>
On Tue, Jan 31, 2017 at 7:17 PM, Richard Smith <<a href="mailto:richardsmith@google.com" target="_blank">richardsmith@google.com</a>> wrote:<br>
> I'm fine with these patches being merged. Hopefully we still have plenty of<br>
> time to shake out any problems between now and the release.<br>
><br>
> On 31 January 2017 at 19:09, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>><br>
> wrote:<br>
>><br>
>> > IIUC the major risk is that diagnose_if itself turns out to be broken,<br>
>> > not that we'd miscompile anything?<br>
>><br>
>> Correct. These patches should be NFC to code that doesn't use diagnose_if.<br>
>><br>
>> If something about that patch *had* to break existing<br>
>> non-diagnose_if-aware code, we're now calling Sema::CheckFunctionCall (which<br>
>> checks diagnose_if attributes, checks we're not passing a nullptr into<br>
>> _Nonnull args, ...) for more kinds of C++ function calls than before. We<br>
>> weren't calling it before in these places as an optimization: in theory, it<br>
>> wasn't possible for CheckFunctionCall to find issues in calls to things like<br>
>> conversion functions. Now that we have diagnose_if, it is.<br>
>><br>
>> On Tue, Jan 31, 2017 at 6:31 PM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
>>><br>
>>> I'm Ok with taking the larger patch (r293360 + r293369) too. It's been<br>
>>> in tree for a bit, there is still a number of weeks before the<br>
>>> release, and IIUC the major risk is that diagnose_if itself turns out<br>
>>> to be broken, not that we'd miscompile anything?<br>
>>><br>
>>> On Tue, Jan 31, 2017 at 11:11 AM, Richard Smith <<a href="mailto:richardsmith@google.com" target="_blank">richardsmith@google.com</a>><br>
>>> wrote:<br>
>>> > Yes, this makes sense. We should also decide what we're going to do<br>
>>> > about<br>
>>> > the larger diagnose_if patch that George has asked to be ported to<br>
>>> > Clang 4.<br>
>>> > Are you comfortable taking a patch of that size? If not, perhaps we<br>
>>> > should<br>
>>> > disable the attribute for the Clang 4 release instead.<br>
>>> ><br>
>>> ><br>
>>> > On 31 January 2017 at 10:36, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>> wrote:<br>
>>> >><br>
>>> >> Ping?<br>
>>> >><br>
>>> >> On Thu, Jan 26, 2017 at 10:21 AM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>><br>
>>> >> wrote:<br>
>>> >> > Ping?<br>
>>> >> ><br>
>>> >> > On Mon, Jan 23, 2017 at 4:27 PM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>><br>
>>> >> > wrote:<br>
>>> >> >> Ping?<br>
>>> >> >><br>
>>> >> >> On Tue, Jan 17, 2017 at 4:16 PM, Hans Wennborg <<a href="mailto:hans@chromium.org" target="_blank">hans@chromium.org</a>><br>
>>> >> >> wrote:<br>
>>> >> >>> Richard, what do you think?<br>
>>> >> >>><br>
>>> >> >>> On Fri, Jan 13, 2017 at 3:16 PM, Eric Fiselier <<a href="mailto:eric@efcs.ca" target="_blank">eric@efcs.ca</a>><br>
>>> >> >>> wrote:<br>
>>> >> >>>> I would love to see this merged. It would make it easier to write<br>
>>> >> >>>> libc++<br>
>>> >> >>>> tests if the tests didn't have to worry about the old 4.0<br>
>>> >> >>>> behavior.<br>
>>> >> >>>><br>
>>> >> >>>> CC'ing Richard: Would merging this be OK?<br>
>>> >> >>>><br>
>>> >> >>>> On Fri, Jan 13, 2017 at 3:46 PM, George Burgess IV<br>
>>> >> >>>> <<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>> wrote:<br>
>>> >> >>>>><br>
>>> >> >>>>> Do we want to consider merging this into the release branch?<br>
>>> >> >>>>> Seems<br>
>>> >> >>>>> like<br>
>>> >> >>>>> more of a bugfix than a feature to me.<br>
>>> >> >>>>><br>
>>> >> >>>>> On Fri, Jan 13, 2017 at 2:11 PM, Eric Fiselier via cfe-commits<br>
>>> >> >>>>> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Author: ericwf<br>
>>> >> >>>>>> Date: Fri Jan 13 16:11:40 2017<br>
>>> >> >>>>>> New Revision: 291963<br>
>>> >> >>>>>><br>
>>> >> >>>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=291963&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=291963&view=rev</a><br>
>>> >> >>>>>> Log:<br>
>>> >> >>>>>> [clang] Emit `diagnose_if` warnings from system headers<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Summary: In order for libc++ to meaningfully use `diagnose_if`<br>
>>> >> >>>>>> warnings<br>
>>> >> >>>>>> they need to be emitted from system headers by default. This<br>
>>> >> >>>>>> patch<br>
>>> >> >>>>>> changes<br>
>>> >> >>>>>> the `diagnose_if` warning diagnostic to be shown in system<br>
>>> >> >>>>>> headers.<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Reviewers: george.burgess.iv, rsmith, aaron.ballman<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Subscribers: cfe-commits<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Differential Revision: <a href="https://reviews.llvm.org/D28703" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2870<wbr>3</a><br>
>>> >> >>>>>><br>
>>> >> >>>>>> Added:<br>
>>> >> >>>>>>     cfe/trunk/test/Sema/Inputs/di<wbr>agnose-if-warn-system-header.h<br>
>>> >> >>>>>> Modified:<br>
>>> >> >>>>>>     cfe/trunk/include/clang/<wbr>Basic/DiagnosticSemaKinds.td<br>
>>> >> >>>>>>     cfe/trunk/test/Sema/diagnose_<wbr>if.c<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Modified: cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
>>> >> >>>>>> URL:<br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=291963&r1=291962&r2=291963&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/include/clang/<wbr>Basic/DiagnosticSemaKinds.td?<wbr>rev=291963&r1=291962&r2=<wbr>291963&view=diff</a><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> ==============================<wbr>==============================<wbr>==================<br>
>>> >> >>>>>> --- cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td<br>
>>> >> >>>>>> (original)<br>
>>> >> >>>>>> +++ cfe/trunk/include/clang/Basic/<wbr>DiagnosticSemaKinds.td Fri<br>
>>> >> >>>>>> Jan 13<br>
>>> >> >>>>>> 16:11:40 2017<br>
>>> >> >>>>>> @@ -3380,7 +3380,8 @@ def note_ovl_candidate_has_pass_ob<wbr>ject_s<br>
>>> >> >>>>>>      "candidate address cannot be taken because parameter %0<br>
>>> >> >>>>>> has "<br>
>>> >> >>>>>>      "pass_object_size attribute">;<br>
>>> >> >>>>>>  def err_diagnose_if_succeeded : Error<"%0">;<br>
>>> >> >>>>>> -def warn_diagnose_if_succeeded : Warning<"%0">,<br>
>>> >> >>>>>> InGroup<UserDefinedWarnings>;<br>
>>> >> >>>>>> +def warn_diagnose_if_succeeded : Warning<"%0">,<br>
>>> >> >>>>>> InGroup<UserDefinedWarnings>,<br>
>>> >> >>>>>> +    ShowInSystemHeader;<br>
>>> >> >>>>>>  def note_ovl_candidate_disabled_by<wbr>_function_cond_attr : Note<<br>
>>> >> >>>>>>      "candidate disabled: %0">;<br>
>>> >> >>>>>>  def note_ovl_candidate_disabled_by<wbr>_extension : Note<<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Added:<br>
>>> >> >>>>>> cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h<br>
>>> >> >>>>>> URL:<br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/Inputs/diagnose-if-warn-system-header.h?rev=291963&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Sema/Inpu<wbr>ts/diagnose-if-warn-system-<wbr>header.h?rev=291963&view=auto</a><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> ==============================<wbr>==============================<wbr>==================<br>
>>> >> >>>>>> --- cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h<br>
>>> >> >>>>>> (added)<br>
>>> >> >>>>>> +++ cfe/trunk/test/Sema/Inputs/dia<wbr>gnose-if-warn-system-header.h<br>
>>> >> >>>>>> Fri<br>
>>> >> >>>>>> Jan<br>
>>> >> >>>>>> 13 16:11:40 2017<br>
>>> >> >>>>>> @@ -0,0 +1,11 @@<br>
>>> >> >>>>>> +#pragma GCC system_header<br>
>>> >> >>>>>> +<br>
>>> >> >>>>>> +inline int system_header_func(int x)<br>
>>> >> >>>>>> +  __attribute__((diagnose_if(x == x, "system header warning",<br>
>>> >> >>>>>> "warning"))) // expected-note {{from 'diagnose_if' attribute}}<br>
>>> >> >>>>>> +{<br>
>>> >> >>>>>> +  return 0;<br>
>>> >> >>>>>> +}<br>
>>> >> >>>>>> +<br>
>>> >> >>>>>> +void test_system_header() {<br>
>>> >> >>>>>> +  system_header_func(0); // expected-warning {{system header<br>
>>> >> >>>>>> warning}}<br>
>>> >> >>>>>> +}<br>
>>> >> >>>>>><br>
>>> >> >>>>>> Modified: cfe/trunk/test/Sema/diagnose_i<wbr>f.c<br>
>>> >> >>>>>> URL:<br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/diagnose_if.c?rev=291963&r1=291962&r2=291963&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/cfe/trunk/test/Sema/diag<wbr>nose_if.c?rev=291963&r1=291962<wbr>&r2=291963&view=diff</a><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> ==============================<wbr>==============================<wbr>==================<br>
>>> >> >>>>>> --- cfe/trunk/test/Sema/diagnose_i<wbr>f.c (original)<br>
>>> >> >>>>>> +++ cfe/trunk/test/Sema/diagnose_i<wbr>f.c Fri Jan 13 16:11:40 2017<br>
>>> >> >>>>>> @@ -150,3 +150,6 @@ void alwaysWarnWithArg(int a) _diagnose_<br>
>>> >> >>>>>>  void runAlwaysWarnWithArg(int a) {<br>
>>> >> >>>>>>    alwaysWarnWithArg(a); // expected-warning{{alwaysWarn}}<br>
>>> >> >>>>>>  }<br>
>>> >> >>>>>> +<br>
>>> >> >>>>>> +// Test that diagnose_if warnings generated in system headers<br>
>>> >> >>>>>> are<br>
>>> >> >>>>>> not<br>
>>> >> >>>>>> ignored.<br>
>>> >> >>>>>> +#include "Inputs/diagnose-if-warn-syste<wbr>m-header.h"<br>
>>> >> >>>>>><br>
>>> >> >>>>>><br>
>>> >> >>>>>> ______________________________<wbr>_________________<br>
>>> >> >>>>>> cfe-commits mailing list<br>
>>> >> >>>>>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>>> >> >>>>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
>>> >> >>>>><br>
>>> >> >>>>><br>
>>> >> >>>><br>
>>> ><br>
>>> ><br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div></div>