[cfe-commits] r124279 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td test/Preprocessor/pragma_diagnostic_sections.cpp test/Sema/uninit-variables.c test/SemaCXX/uninit-variables.cpp

[Nico Weber]

On Mon, Jan 31, 2011 at 3:43 AM, Chandler Carruth <chandlerc at google.com> wrote:
> Ted, much like Nico, I'm not really happy with this change.

Just for the record, I think the change is pretty cool (-Wunitialized
at -O0, OMG!). The code is new, so there are issues, and a good way to
find these is to turn it on by default and see who complains. I filed
bugs for the most common problems we have with this in chrome, so this
seems to be working :-)

Nico

(I did disable -Wuninitialized for chrome for now. Our other compilers
will catch most problems…but I think only clang warns about |A(int m)
: m_(m_) {}|, so I hope I can turn it back on soon.)

(I failed to send this to the list for some reason – sorry for the
duplicate mail, Ted & Chandler)

>
> 1) This is a major change in policy about -Wuninitialized that was not
> discussed previously on cfe-dev. Can we have a discussion there?
> Specifically, you cite goals for the flag that I can't find either
> documented or discussed anywhere. I'm not saying I disagree with your
> statements, just that it comes a bit out of the blue.
>
> 2) I think that folding all of these warnings under one flag is very
> problematic. At Google, we've struggled greatly to find even a
> somewhat reasonable way to warn about uninitialized variables, and
> while I'm not fond of the GCC model, I'm not very fond of this one
> either. The current state makes it impossible for us to turn on
> -Wuninitialized (I'm having trouble even *counting* the times it fires
> in our codebase because every build dies so early), which is a serious
> regression. Previously, we had this flag on and it caught real bugs
> with zero false positives. Now those bugs will slip through.
>
> I'd propose first moving this new functionality back under a separate
> flag, but perhaps one less scary than -Wuninitialized-experimental
> (maybe -Wmaybe-uninitialized? That flag was added to GCC recently
> based on our experience IIRC...), to avoid regression for large
> codebases currently using Clang. Then let's discuss in detail exactly
> how to balance the tradeoffs between false positives and reasonable
> analyses on the main mailing list, and how to break down these types
> of features. I don't want to dig into that here, as I think a wider
> audience would be better.
>
> Just to be clear, I'm not at all opposed to this type of
> functionality, but I'm a bit hesitant to turn it on w/o thinking
> carefully about it, and finding good ways to control the various
> different categories of warnings so that different codebases with
> different tolerances for these warnings can get the most out of Clang.
> I'm also somewhat allergic to regressions. ;]
>
> On Tue, Jan 25, 2011 at 8:49 PM, Ted Kremenek <kremenek at apple.com> wrote:
>> Author: kremenek
>> Date: Tue Jan 25 22:49:48 2011
>> New Revision: 124279
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=124279&view=rev
>> Log:
>> Merge -Wuninitialized-experimental into -Wuninitialized.
>>
>> Modified:
>>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>    cfe/trunk/test/Preprocessor/pragma_diagnostic_sections.cpp
>>    cfe/trunk/test/Sema/uninit-variables.c
>>    cfe/trunk/test/SemaCXX/uninit-variables.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=124279&r1=124278&r2=124279&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Jan 25 22:49:48 2011
>> @@ -821,9 +821,9 @@
>>  def note_uninit_reference_member : Note<
>>   "uninitialized reference member is here">;
>>  def warn_field_is_uninit : Warning<"field is uninitialized when used here">,
>> -  InGroup<DiagGroup<"uninitialized">>;
>> +  InGroup<Uninitialized>;
>>  def warn_uninit_var : Warning<"use of uninitialized variable %0">,
>> -  InGroup<DiagGroup<"uninitialized-experimental">>, DefaultIgnore;
>> +  InGroup<Uninitialized>, DefaultIgnore;
>>  def note_uninit_var : Note<
>>   "variable %0 is possibly uninitialized when used here">;
>>  def note_uninit_var_captured_by_block : Note<
>>
>> Modified: cfe/trunk/test/Preprocessor/pragma_diagnostic_sections.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/pragma_diagnostic_sections.cpp?rev=124279&r1=124278&r2=124279&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Preprocessor/pragma_diagnostic_sections.cpp (original)
>> +++ cfe/trunk/test/Preprocessor/pragma_diagnostic_sections.cpp Tue Jan 25 22:49:48 2011
>> @@ -1,4 +1,4 @@
>> -// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -verify %s
>> +// RUN: %clang_cc1 -fsyntax-only -Wall -Wunused-macros -Wunused-parameter -Wno-uninitialized -verify %s
>>
>>  // rdar://8365684
>>  struct S {
>>
>> Modified: cfe/trunk/test/Sema/uninit-variables.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/uninit-variables.c?rev=124279&r1=124278&r2=124279&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/Sema/uninit-variables.c (original)
>> +++ cfe/trunk/test/Sema/uninit-variables.c Tue Jan 25 22:49:48 2011
>> @@ -1,4 +1,4 @@
>> -// RUN: %clang_cc1 -fsyntax-only -Wuninitialized-experimental -fsyntax-only -fblocks %s -verify
>> +// RUN: %clang_cc1 -fsyntax-only -Wuninitialized -fsyntax-only -fblocks %s -verify
>>
>>  int test1() {
>>   int x; // expected-warning{{use of uninitialized variable 'x'}} expected-note{{add initialization to silence this warning}}
>>
>> Modified: cfe/trunk/test/SemaCXX/uninit-variables.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/uninit-variables.cpp?rev=124279&r1=124278&r2=124279&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/SemaCXX/uninit-variables.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/uninit-variables.cpp Tue Jan 25 22:49:48 2011
>> @@ -1,4 +1,4 @@
>> -// RUN: %clang_cc1 -fsyntax-only -Wuninitialized-experimental -fsyntax-only %s -verify
>> +// RUN: %clang_cc1 -fsyntax-only -Wuninitialized -fsyntax-only %s -verify
>>
>>  int test1_aux(int &x);
>>  int test1() {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>