[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

[Chandler Carruth]

Ted, much like Nico, I'm not really happy with this change.

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
>