[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

2019-03-16 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert updated this revision to Diff 190966.
aaronpuchert added a comment.

Don't suggest adding `static` if there is a non-prototype declaration. This 
required a minor refactoring: we let `ShouldWarnAboutMissingPrototype` return 
any kind of declaration it finds and check for the number of parameters at the 
call site.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59402/new/

https://reviews.llvm.org/D59402

Files:
  lib/Sema/SemaDecl.cpp
  test/Sema/warn-missing-prototypes.c
  test/Sema/warn-missing-variable-declarations.c
  test/SemaCXX/warn-missing-variable-declarations.cpp

Index: test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- test/SemaCXX/warn-missing-variable-declarations.cpp
+++ test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -1,11 +1,15 @@
-// RUN: %clang -Wmissing-variable-declarations -fsyntax-only -Xclang -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-variable-declarations -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits -std=c++17 %s 2>&1 | FileCheck %s
 
 // Variable declarations that should trigger a warning.
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 namespace x {
   int vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"static "
 }
 
 // Variable declarations that should not trigger a warning.
@@ -58,7 +62,9 @@
 constexpr int constexpr_var = 0;
 inline constexpr int inline_constexpr_var = 0;
 extern const int extern_const_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 template int var_template = 0;
 template constexpr int const_var_template = 0;
Index: test/Sema/warn-missing-variable-declarations.c
===
--- test/Sema/warn-missing-variable-declarations.c
+++ test/Sema/warn-missing-variable-declarations.c
@@ -1,6 +1,8 @@
 // RUN: %clang_cc1 -Wmissing-variable-declarations -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 int vbad2;
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
@@ -8,6 +10,7 @@
 struct {
   int mgood1;
 } vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:1-[[@LINE-3]]:1}:"static "
 
 int vbad4;
 int vbad4 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad4'}}
Index: test/Sema/warn-missing-prototypes.c
===
--- test/Sema/warn-missing-prototypes.c
+++ test/Sema/warn-missing-prototypes.c
@@ -4,15 +4,20 @@
 int f();
 
 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 static int g(int x) { return x; }
 
 int h(int x) { return x; } // expected-warning{{no previous prototype for function 'h'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 static int g2();
 
 int g2(int x) { return x; }
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+
 void test(void);
 
 int h3();
@@ -40,4 +45,4 @@
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}
 
-// CHECK: fix-it:"{{.*}}":{40:27-40:27}:"void"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:27-[[@LINE-3]]:27}:"void"
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11805,7 +11805,11 @@
   prev = prev->getPreviousDecl();
 
 if (!prev)
-  Diag(var->getLocation(), diag::warn_missing_variable_declarations) << var;
+  Diag(var->getLocation(), diag::warn_missing_variable_declarations)
+  << 

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

2019-03-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert added inline comments.



Comment at: test/Sema/warn-missing-prototypes.c:7
 int f(int x) { return x; } // expected-warning{{no previous prototype for 
function 'f'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 

Maybe there shouldn't be a fix-it here, as there is a declaration, which is 
just not a prototype.



Comment at: test/Sema/warn-missing-prototypes.c:17-18
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype 
for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+

As I wrote in the commit message: this could be fixed by replacing `extern` 
with `static`, but I don't think we have the `SourceLocation` for the storage 
class specifier available where the warning is emitted. Maybe I'm overlooking 
something though.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59402/new/

https://reviews.llvm.org/D59402



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

2019-03-14 Thread Aaron Puchert via Phabricator via cfe-commits
aaronpuchert created this revision.
aaronpuchert added reviewers: bkramer, efriedma, rsmith.
Herald added subscribers: cfe-commits, jdoerfert.
Herald added a project: clang.

I've found that most often the proper way to fix this warning is to add
`static`, because if the code otherwise compiles and links, the function
or variable is apparently not needed outside of the TU.

There is no fix-it for the rare case of an "extern definition", because
that would require removing `extern` and I have no idea how to get the
source location of the storage class specifier from a VarDecl. I believe
this information is only available earlier in the AST construction from
DeclSpec::getStorageClassSpecLoc(), but we don't have that here.


Repository:
  rC Clang

https://reviews.llvm.org/D59402

Files:
  lib/Sema/SemaDecl.cpp
  test/Sema/warn-missing-prototypes.c
  test/Sema/warn-missing-variable-declarations.c
  test/SemaCXX/warn-missing-variable-declarations.cpp

Index: test/SemaCXX/warn-missing-variable-declarations.cpp
===
--- test/SemaCXX/warn-missing-variable-declarations.cpp
+++ test/SemaCXX/warn-missing-variable-declarations.cpp
@@ -1,11 +1,15 @@
-// RUN: %clang -Wmissing-variable-declarations -fsyntax-only -Xclang -verify -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wmissing-variable-declarations -std=c++17 %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits -std=c++17 %s 2>&1 | FileCheck %s
 
 // Variable declarations that should trigger a warning.
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 namespace x {
   int vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:3-[[@LINE-1]]:3}:"static "
 }
 
 // Variable declarations that should not trigger a warning.
@@ -58,7 +62,9 @@
 constexpr int constexpr_var = 0;
 inline constexpr int inline_constexpr_var = 0;
 extern const int extern_const_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 extern constexpr int extern_constexpr_var = 0; // expected-warning {{no previous extern declaration}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
 
 template int var_template = 0;
 template constexpr int const_var_template = 0;
Index: test/Sema/warn-missing-variable-declarations.c
===
--- test/Sema/warn-missing-variable-declarations.c
+++ test/Sema/warn-missing-variable-declarations.c
@@ -1,6 +1,8 @@
 // RUN: %clang_cc1 -Wmissing-variable-declarations -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmissing-variable-declarations -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
 
 int vbad1; // expected-warning{{no previous extern declaration for non-static variable 'vbad1'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 int vbad2;
 int vbad2 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad2'}}
@@ -8,6 +10,7 @@
 struct {
   int mgood1;
 } vbad3; // expected-warning{{no previous extern declaration for non-static variable 'vbad3'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:1-[[@LINE-3]]:1}:"static "
 
 int vbad4;
 int vbad4 = 10; // expected-warning{{no previous extern declaration for non-static variable 'vbad4'}}
Index: test/Sema/warn-missing-prototypes.c
===
--- test/Sema/warn-missing-prototypes.c
+++ test/Sema/warn-missing-prototypes.c
@@ -4,6 +4,7 @@
 int f();
 
 int f(int x) { return x; } // expected-warning{{no previous prototype for function 'f'}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:1-[[@LINE-1]]:1}:"static "
 
 static int g(int x) { return x; }
 
@@ -13,6 +14,9 @@
 
 int g2(int x) { return x; }
 
+extern int g3(int x) { return x; } // expected-warning{{no previous prototype for function 'g3'}}
+// CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]{{.*}}}:"{{.*}}"
+
 void test(void);
 
 int h3();
@@ -40,4 +44,4 @@
 void not_a_prototype_test(); // expected-note{{this declaration is not a prototype; add 'void' to make it a prototype for a zero-parameter function}}
 void not_a_prototype_test() { } // expected-warning{{no previous prototype for function 'not_a_prototype_test'}}
 
-// CHECK: fix-it:"{{.*}}":{40:27-40:27}:"void"
+// CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:27-[[@LINE-3]]:27}:"void"
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -11805,7 +11805,11 @@
   prev = prev->getPreviousDecl();
 
 if