[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

A couple of late comments.




Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:39
+  return;
+} else if (DefaultArgRange.getBegin().isMacroID()) {
+  diag(D->getLocStart(),

No `else` after `return`, please. 
http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:56
+  "declaring a parameter with a default argument is disallowed")
+  << D
+  << FixItHint::CreateRemoval(RemovalRange);

This argument is not used in the message. Does it have any effect at all?



Comment at: docs/clang-tidy/index.rst:64
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.

How about adding a link to the coding style document? (We should do this for 
other entries here, but that's unrelated to your patch.)


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D40108#938131, @juliehockett wrote:

> If you could, that'd be great! Thank you!


It'll be good idea it you'll request commit rights, since you have more checks 
in development queue.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit in r319225.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

If you could, that'd be great! Thank you!


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for the nit fix. If you'd like me to commit this on your behalf, just 
let me know.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from one small nit with the test file missing a trailing newline.




Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:82
+}
\ No newline at end of file


Please add a newline at the end of the file.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 124421.
juliehockett marked an inline comment as done.
juliehockett added a comment.

Updating docs


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: int foo(int value) { return value; }
+
+int f() {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+int bar(int value) { return value; }
+
+int n() {
+  foo(0);
+  bar(0);
+}
+
+class Baz {
+public:
+  int a(int value = 5) { return value; }
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value) { return value; }
+
+  int b(int value) { return value; }
+};
+
+class Foo {
+  // Fix should be suggested in declaration
+  int a(int value = 53);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value);
+};
+
+// Fix shouldn't be suggested in implementation
+int Foo::a(int value) {
+  return value;
+}
+
+// Elided functions
+void f(int = 5) {};
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void f(int) {};
+
+void g(int) {};
+
+// Should not suggest fix for macro-defined parameters
+#define D(val) = val
+
+void h(int i D(5));
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES-NOT: void h(int i);
+
+void x(int i);
+void x(int i = 12);
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void x(int i);
+
+void x(int i) {}
+
+struct S {
+  void x(int i);
+};
+
+void S::x(int i = 12) {}
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void S::x(int i) {}
+
+int main() {
+  S s;
+  s.x();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: void S::x(int i = 12) {}
+  x();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: void x(int i = 12);
+}
\ No newline at end of file
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -55,6 +55,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - 

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:6
+
+Warns if a function is declared or called with default arguments.
+

Please synchronize with text in Release Notes.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 124397.
juliehockett marked 7 inline comments as done.
juliehockett added a comment.

Added new tests and updated wording in warning.


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,81 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: int foo(int value) { return value; }
+
+int f() {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+int bar(int value) { return value; }
+
+int n() {
+  foo(0);
+  bar(0);
+}
+
+class Baz {
+public:
+  int a(int value = 5) { return value; }
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value) { return value; }
+
+  int b(int value) { return value; }
+};
+
+class Foo {
+  // Fix should be suggested in declaration
+  int a(int value = 53);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value);
+};
+
+// Fix shouldn't be suggested in implementation
+int Foo::a(int value) {
+  return value;
+}
+
+// Elided functions
+void f(int = 5) {};
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void f(int) {};
+
+void g(int) {};
+
+// Should not suggest fix for macro-defined parameters
+#define D(val) = val
+
+void h(int i D(5));
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES-NOT: void h(int i);
+
+void x(int i);
+void x(int i = 12);
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void x(int i);
+
+void x(int i) {}
+
+struct S {
+  void x(int i);
+};
+
+void S::x(int i = 12) {}
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void S::x(int i) {}
+
+int main() {
+  S s;
+  s.x();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: void S::x(int i = 12) {}
+  x();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: default parameter was declared here:
+  // CHECK-NEXT: void x(int i = 12);
+}
\ No newline at end of file
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -55,6 +55,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,24 @@

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38
+SourceRange RemovalRange(
+  Lexer::getLocForEndOfToken(D->getLocation(), 0, 
+*Result.SourceManager, Result.Context->getLangOpts()),
+  D->getDefaultArgRange().getEnd()

juliehockett wrote:
> aaron.ballman wrote:
> > Does `getDefaultArgRange()` not provide the correct range already (so you 
> > don't need to go back to the original source)? Or does that range miss the 
> > `=`?
> > 
> > You might want to disable the fix-it in the case `getDefaultArgRange()` 
> > returns an empty range, or in case the default arg expression comes from a 
> > macro (and add a test for the latter case). e.g.,
> > ```
> > #define DERP(val)  = val
> > 
> > void f(int i DERP);
> > ```
> > Additionally, a test where the identifier is elided would be good as well: 
> > `void f(int = 12);`
> `getDefaultArgRange()` misses the `=` -- though if there's a better way to do 
> it I'm all ears! 
In that case, this seems fine to me.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:31
+diag(S->getParam()->getLocStart(),
+"the default parameter was declared here",
+ DiagnosticIDs::Note);

Drop "the" from the diagnostic to match wording in other places



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:44
+  diag(DefaultArgRange.getBegin(),
+"the default parameter was declared here",
+DiagnosticIDs::Note);

Drop "the" here as well.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:48-52
+  if (!D->getName().empty()) {
+   StartLocation = D->getLocation();
+  } else {
+   StartLocation = D->getLocStart();
+  }

This can be hoisted to the initialization: `SourceLocation StartLocation = 
D->getName().empty() ? D->getLocStart() : D->getLocation();`



Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:40
+  return value;
+  // CHECK-MESSAGES: [[@LINE-2]]:12: warning: declaring a parameter with a 
default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:

I think this diagnostic should be suppressed -- having it on the declaration 
where the default argument is defined should be sufficient, no?



Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:57
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a 
default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES-NOT: void h(int i);

Two more test cases that should be added:
```
void f(int i);
void f(int i = 12);

void f(int i) {

}

struct S {
  void f(int i);
};

void S::f(int i = 12) {}

int main() {
  S s;
  s.f();
  f();
}
```


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 123857.
juliehockett marked 6 inline comments as done.
juliehockett added a comment.

Disabled the fix-it for `getDefaultArgRange()` that returns an empty range and 
for default args that come from a macro.


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,57 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: int foo(int value) { return value; }
+
+int f() {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling a function that uses a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+int bar(int value) { return value; }
+
+int n() {
+  foo(0);
+  bar(0);
+}
+
+class Baz {
+public:
+  int a(int value = 5) { return value; }
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value) { return value; }
+
+  int b(int value) { return value; }
+};
+
+class Foo {
+  // Fix should be suggested in declaration
+  int a(int value = 53);
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value);
+};
+
+// Fix shouldn't be suggested in implementation
+int Foo::a(int value) {
+  return value;
+  // CHECK-MESSAGES: [[@LINE-2]]:12: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int a(int value = 53);
+}
+
+// Elided functions
+void f(int = 5) {};
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: void f(int) {};
+
+void g(int) {};
+
+// Should not suggest fix for macro-defined parameters
+#define D(val) = val
+
+void h(int i D(5));
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: declaring a parameter with a default argument is disallowed [fuchsia-default-arguments]
+// CHECK-FIXES-NOT: void h(int i);
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -55,6 +55,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,24 @@
+.. title:: clang-tidy - fuchsia-default-arguments
+
+fuchsia-default-arguments
+=
+
+Warns if a function is declared or called with default arguments.
+
+For example, the declaration:
+
+.. code-block:: c++
+
+  int foo(int value = 5) { return value; }
+
+will cause a warning.
+
+A function call expression that uses a default argument will be diagnosed.
+Calling it without defaults will not cause a warning:
+
+.. code-block:: c++
+
+  foo();  // warning
+  foo(0); // no warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
Index: docs/ReleaseNotes.rst

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

We're going to go with fuchsia-* for the module name, since the style applies 
to the broader project. Also, there may be zircon-specific checks at some 
point, so we want to leave the door open for that.




Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38
+SourceRange RemovalRange(
+  Lexer::getLocForEndOfToken(D->getLocation(), 0, 
+*Result.SourceManager, Result.Context->getLangOpts()),
+  D->getDefaultArgRange().getEnd()

aaron.ballman wrote:
> Does `getDefaultArgRange()` not provide the correct range already (so you 
> don't need to go back to the original source)? Or does that range miss the 
> `=`?
> 
> You might want to disable the fix-it in the case `getDefaultArgRange()` 
> returns an empty range, or in case the default arg expression comes from a 
> macro (and add a test for the latter case). e.g.,
> ```
> #define DERP(val)  = val
> 
> void f(int i DERP);
> ```
> Additionally, a test where the identifier is elided would be good as well: 
> `void f(int = 12);`
`getDefaultArgRange()` misses the `=` -- though if there's a better way to do 
it I'm all ears! 


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Did you decide on fuchsia instead of zircon for the module name, or is that 
decision still up in the air?




Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22
+  // Calling a function which uses default arguments is disallowed.
+  Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this);
+  // Declaring default parameters is disallowed.

juliehockett wrote:
> aaron.ballman wrote:
> > Isn't this case already covered by the other matcher? Or do you have system 
> > header files which have default arguments and those functions are 
> > disallowed by the style guide, but cannot be removed from the system header?
> The latter -- there's a bunch of third-party code and the like that could 
> have default arguments, but shouldn't be used like that in the project.
Ah, that makes sense. Thanks!



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:29
+diag(S->getUsedLocation(),
+ "calling functions which use default arguments are disallowed");
+diag(S->getParam()->getLocStart(), 

I think the diagnostic should be in singular form. How about: `calling a 
function that uses a default argument is disallowed`?



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:35-38
+SourceRange RemovalRange(
+  Lexer::getLocForEndOfToken(D->getLocation(), 0, 
+*Result.SourceManager, Result.Context->getLangOpts()),
+  D->getDefaultArgRange().getEnd()

Does `getDefaultArgRange()` not provide the correct range already (so you don't 
need to go back to the original source)? Or does that range miss the `=`?

You might want to disable the fix-it in the case `getDefaultArgRange()` returns 
an empty range, or in case the default arg expression comes from a macro (and 
add a test for the latter case). e.g.,
```
#define DERP(val)  = val

void f(int i DERP);
```
Additionally, a test where the identifier is elided would be good as well: 
`void f(int = 12);`



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:41
+diag(D->getLocStart(),
+ "declaring functions which use default arguments are disallowed")
+ << D

This diagnostic could be similarly tightened -- the function isn't the issue, 
the parameter with the default argument is. Perhaps: `declaring a parameter 
with a default value is disallowed`?



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:16-17
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:

The wording here is a bit confusing; it's not that calling it with no arguments 
causes the warning, it's that calling it such that the default argument value 
is used that's the problem. Perhaps: `A function call expression that uses a 
default argument will be diagnosed.` or something along those lines.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 123677.
juliehockett marked 8 inline comments as done.
juliehockett added a comment.

Moved AST matcher out to ASTMatchers.h (see D40261 
), fixing comments


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,29 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments are disallowed [fuchsia-default-arguments]
+// CHECK-FIXES: int foo(int value) { return value; }
+
+int f() {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling functions which use default arguments are disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+// Negatives.
+int bar(int value) { return value; }
+
+int n() {
+  foo(0);
+  bar(0);
+}
+
+class Baz {
+public:
+  int a(int value = 5) { return value; }
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments are disallowed [fuchsia-default-arguments]
+  // CHECK-FIXES: int a(int value) { return value; }
+  
+  int b(int value) { return value; }
+};
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -55,6 +55,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - fuchsia-default-arguments
+
+fuchsia-default-arguments
+=
+
+Warns if a function is declared or called with default arguments.
+
+For example, the declaration:
+
+.. code-block:: c++
+
+  int foo(int value = 5) { return value; }
+
+will cause a warning.
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:
+
+.. code-block:: c++
+
+  foo();  // warning
+  foo(0); // no warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `fuchsia-default-arguments
+  `_ check
+
+  Warns if a function or method is declared or called with default arguments.
+
 - New `google-avoid-throwing-objc-exception
   `_ check
 
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -482,6 +482,11 @@
 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
 CppCoreGuidelinesModuleAnchorSource;
 
+// This anchor is used to force the linker to link the GoogleModule.
+extern volatile int FuchsiaModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED FuchsiaModuleAnchorDestination =
+FuchsiaModuleAnchorSource;

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added inline comments.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22
+  // Calling a function which uses default arguments is disallowed.
+  Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this);
+  // Declaring default parameters is disallowed.

aaron.ballman wrote:
> Isn't this case already covered by the other matcher? Or do you have system 
> header files which have default arguments and those functions are disallowed 
> by the style guide, but cannot be removed from the system header?
The latter -- there's a bunch of third-party code and the like that could have 
default arguments, but shouldn't be used like that in the project.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

Thank you for the explanation about what is driving this and the documentation. 
I don't think google-* is the correct module for this as it's too general of an 
umbrella. I have a slight preference for zircon-* because this appears to be 
specific to that particular project.




Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:18
+
+AST_MATCHER(ParmVarDecl, hasDefaultArgument) { return Node.hasDefaultArg(); }
+

I think this should be put into ASTMatchers.h as a separate patch (feel free to 
list me as a reviewer). See D39940 for an example of how to do that, but the 
only non-obvious task is running clang\docs\tools\dump_ast_matchers.py to 
generate the HTML from ASTMatchers.h.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:22
+  // Calling a function which uses default arguments is disallowed.
+  Finder->addMatcher(cxxDefaultArgExpr().bind("stmt"), this);
+  // Declaring default parameters is disallowed.

Isn't this case already covered by the other matcher? Or do you have system 
header files which have default arguments and those functions are disallowed by 
the style guide, but cannot be removed from the system header?



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:28
+void DefaultArgumentsCheck::check(const MatchFinder::MatchResult ) {
+  if (const CXXDefaultArgExpr *S =
+  Result.Nodes.getNodeAs("stmt")) {

You can use `const auto *` here instead of spelling the type out twice.



Comment at: clang-tidy/fuchsia/DefaultArgumentsCheck.cpp:36
+diag(D->getLocStart(),
+ "declaring functions which use default arguments is disallowed");
+  }

Would it make sense to add a fix-it that removes the intializer for the default 
argument?



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:8
+
+Example: The declaration:
+

Eugene.Zelenko wrote:
> I briefly look on other checks documentation, so will be good idea to use 
> just //Example:// or //For example, the declaration:// . But will be good 
> idea to hear opinion of native English speaker.
I'd go with: `For example, the declaration:`



Comment at: docs/clang-tidy/index.rst:672
   will run clang-format over changed lines.
-

Spurious newline removal?


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:63
+
+  Warns if a function is declared or called with default arguments.
+

I think will be good idea to change to //function// to //function or method//. 
Same in documentation.



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:8
+
+Example: The declaration:
+

I briefly look on other checks documentation, so will be good idea to use just 
//Example:// or //For example, the declaration:// . But will be good idea to 
hear opinion of native English speaker.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett added a comment.

In https://reviews.llvm.org/D40108#928224, @Eugene.Zelenko wrote:

>




> I think it should use project-specific prefix, since it's open source 
> project. Google may have different coding guidelines for other projects.

Reasonable. It makes sense to consider it on a check-by-check basis too, I'd 
think.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 123446.
juliehockett marked 2 inline comments as done.
juliehockett added a comment.

Updated docs and added tests to check class methods.


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments is disallowed [fuchsia-default-arguments]
+
+int f() {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling functions which use default arguments is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+// Negatives.
+int bar(int value) { return value; }
+
+int n() {
+  foo(0);
+  bar(0);
+}
+
+class Baz {
+public:
+  int a(int value = 5) { return value; }
+  // CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments is disallowed [fuchsia-default-arguments]
+
+  int b(int value) { return value; }
+};
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
@@ -669,4 +670,3 @@
 * To apply suggested fixes ``-fix`` can be passed as an argument. This gathers
   all changes in a temporary directory and applies them. Passing ``-format``
   will run clang-format over changed lines.
-
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - fuchsia-default-arguments
+
+fuchsia-default-arguments
+=
+
+Warns if a function is declared or called with default arguments.
+
+Example: The declaration:
+
+.. code-block:: c++
+
+  int foo(int value = 5) { return value; }
+
+will cause a warning.
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:
+
+.. code-block:: c++
+
+  foo();  // warning
+  foo(0); // no warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `fuchsia-default-arguments
+  `_ check
+
+  Warns if a function is declared or called with default arguments.
+
 - New `objc-property-declaration
   `_ check
 
@@ -67,8 +72,8 @@
 - New `google-objc-global-variable-declaration
   `_ check
 
-  Add new check for Objective-C code to ensure global 
-  variables follow the naming convention of 'k[A-Z].*' (for constants) 
+  Add new check for Objective-C code to ensure global
+  variables follow the naming convention of 'k[A-Z].*' (for constants)
   or 'g[A-Z].*' (for variables).
 
 - New module `objc` for Objective-C style checks.
@@ 

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D40108#928212, @juliehockett wrote:

> We do need to figure out what the right prefix for these checks is (whether 
> fuchsia-* since they'll be used under the Fuchsia umbrella, zircon-* since 
> the follow the Zircon section of the style guide linked above, or the more 
> generic google-*).


I think it should use project-specific prefix, since it's open source project. 
Google may have different coding guidelines for other projects.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Does Fuchsia coding conventions allows default parameters in class methods? If 
not, test should reflect this




Comment at: docs/ReleaseNotes.rst:63
+
+  Prevent use of default arguments in declared or called functions in Fuchsia.
+

I think will be good idea to have description same as first sentence in 
documentation, i. e. //Warns if a function is declared or called with default 
arguments.//



Comment at: test/clang-tidy/fuchsia-default-arguments.cpp:6
+
+int f(void) {
+  foo();

Please remove //void// (see modernize-redundant-void-arg). Same below.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett marked 2 inline comments as done.
juliehockett added a comment.

We do need to figure out what the right prefix for these checks is (whether 
fuchsia-* since they'll be used under the Fuchsia umbrella, zircon-* since the 
follow the Zircon section of the style guide linked above, or the more generic 
google-*).


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 123278.
juliehockett edited the summary of this revision.
juliehockett added a comment.

Updated docs


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments is disallowed [fuchsia-default-arguments]
+
+int f(void) {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling functions which use default arguments is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+// Negatives.
+int bar(int value) { return value; }
+
+int n(void) {
+  foo(0);
+  bar(0);
+}
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
@@ -669,4 +670,3 @@
 * To apply suggested fixes ``-fix`` can be passed as an argument. This gathers
   all changes in a temporary directory and applies them. Passing ``-format``
   will run clang-format over changed lines.
-
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - fuchsia-default-arguments
+
+fuchsia-default-arguments
+=
+
+Warns if a function is declared or called with default arguments.
+
+Example: The declaration:
+
+.. code-block:: c++
+
+  int foo(int value = 5) { return value; }
+
+will cause a warning.
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:
+
+.. code-block:: c++
+
+  foo();  // warning
+  foo(0); // no warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `fuchsia-default-arguments
+  `_ check
+
+  Prevent use of default arguments in declared or called functions in Fuchsia.
+
 - New `objc-property-declaration
   `_ check
 
@@ -67,8 +72,8 @@
 - New `google-objc-global-variable-declaration
   `_ check
 
-  Add new check for Objective-C code to ensure global 
-  variables follow the naming convention of 'k[A-Z].*' (for constants) 
+  Add new check for Objective-C code to ensure global
+  variables follow the naming convention of 'k[A-Z].*' (for constants)
   or 'g[A-Z].*' (for variables).
 
 - New module `objc` for Objective-C style checks.
@@ -137,21 +142,21 @@
   Finds cases where integer division in a floating point context is likely to
   cause unintended loss of precision.
 
-- New `cppcoreguidelines-owning-memory `_ 

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:60
 
+  Prevent use of default arguments in declared or called functions in Fuchsia.
+

Check name and link was accidentally removed.



Comment at: docs/clang-tidy/checks/list.rst:58
+   fuchsia-default-arguments
+   fuchsia-multiple-inheritance
+   fuchsia-overloaded-operator

List should be pruned.


https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett updated this revision to Diff 123231.
juliehockett marked an inline comment as done.
juliehockett edited the summary of this revision.

https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/fuchsia-default-arguments.cpp

Index: test/clang-tidy/fuchsia-default-arguments.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-default-arguments.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s fuchsia-default-arguments %t
+
+int foo(int value = 5) { return value; }
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: declaring functions which use default arguments is disallowed [fuchsia-default-arguments]
+
+int f(void) {
+  foo();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: calling functions which use default arguments is disallowed [fuchsia-default-arguments]
+  // CHECK-NEXT: note: the default parameter was declared here:
+  // CHECK-NEXT: int foo(int value = 5) { return value; }
+}
+
+// Negatives.
+int bar(int value) { return value; }
+
+int n(void) {
+  foo(0);
+  bar(0);
+}
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -61,6 +61,7 @@
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``fuchsia-``   Checks related to Fuchsia coding conventions.
 ``google-``Checks related to Google coding conventions.
 ``hicpp-`` Checks related to High Integrity C++ Coding Standard.
 ``llvm-``  Checks related to the LLVM coding conventions.
@@ -669,4 +670,3 @@
 * To apply suggested fixes ``-fix`` can be passed as an argument. This gathers
   all changes in a temporary directory and applies them. Passing ``-format``
   will run clang-format over changed lines.
-
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,13 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
+   fuchsia-default-arguments
+   fuchsia-multiple-inheritance
+   fuchsia-overloaded-operator
+   fuchsia-statically-constructed-objects
+   fuchsia-thread-local
+   fuchsia-trailing-return
+   fuchsia-virtual-inheritance
google-build-explicit-make-pair
google-build-namespaces
google-build-using-namespace
Index: docs/clang-tidy/checks/fuchsia-default-arguments.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/fuchsia-default-arguments.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - fuchsia-default-arguments
+
+fuchsia-default-arguments
+=
+
+Warns if a function is declared or called with default arguments.
+
+Example: The declaration:
+
+.. code-block:: c++
+
+  int foo(int value = 5) { return value; }
+
+will cause a warning.
+
+If a function with default arguments is already defined, calling it with no
+arguments will also cause a warning. Calling it without defaults will not cause
+a warning:
+
+.. code-block:: c++
+
+  foo();  // warning
+  foo(0); // no warning
+
+See the features disallowed in Fuchsia at https://fuchsia.googlesource.com/zircon/+/master/docs/cxx.md
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,8 @@
 Improvements to clang-tidy
 --
 
+  Prevent use of default arguments in declared or called functions in Fuchsia.
+
 - New `objc-property-declaration
   `_ check
 
@@ -67,8 +69,8 @@
 - New `google-objc-global-variable-declaration
   `_ check
 
-  Add new check for Objective-C code to ensure global 
-  variables follow the naming convention of 'k[A-Z].*' (for constants) 
+  Add new check for Objective-C code to ensure global
+  variables follow the naming convention of 'k[A-Z].*' (for constants)
   or 'g[A-Z].*' (for variables).
 
 - New module `objc` for Objective-C style checks.
@@ -137,21 +139,21 @@
   Finds cases where integer division in a floating point context is likely to
   cause unintended loss of precision.
 
-- New `cppcoreguidelines-owning-memory 

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

For background: what is Fuchsia and where do these requirements come from (are 
they documented publicly somewhere)?

We tend to prefer concise patches over code dumps, so I think it would make 
sense to split this review into multiple patches. The first one can be adding 
the module and a single check along with background information on why the 
module would be appropriate. The rest of the patches should be one check per 
patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please mention new module in docs/clang-tidy/index.rst.


Repository:
  rL LLVM

https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/fuchsia/FuchsiaTidyModule.cpp:31
+  void addCheckFactories(ClangTidyCheckFactories ) override {
+
+CheckFactories.registerCheck(

Please remove empty line.



Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:100
+Result.Nodes.getNodeAs("decl")) {
+
+// Check against map to see if if the class inherits from multiple 

Please remove empty line.



Comment at: docs/ReleaseNotes.rst:63
+
+  Check to prevent multiple inheritance in Fuchsia.
+

//Check to// should be removed, just //Prevent//. Same in other places.


Repository:
  rL LLVM

https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Please split this review with one check per review.




Comment at: docs/ReleaseNotes.rst:83
+
+  Check to prevent creation of  statically-stored objects in Fuchsia.
+

Please fix double space



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:8
+
+Ex. The declaration:
+

Please don't use abbreviations. Same in other places.



Comment at: docs/clang-tidy/checks/fuchsia-default-arguments.rst:19
+a warning:
+.. code-block:: c++
+

Please insert empty line above.



Comment at: docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst:7
+Warns if statically-stored objects are created, unless constructed
+with constexpr.
+

Please highlight constexpr with ``. Same in other places.



Comment at: docs/clang-tidy/checks/fuchsia-thread-local.rst:6
+
+Warns if thread-local storage is used.
+

Please highlight thread-local and extern with ``. Same in other places.


Repository:
  rL LLVM

https://reviews.llvm.org/D40108



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

[PATCH] D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy

[Julie Hockett via Phabricator via cfe-commits]

juliehockett created this revision.
juliehockett added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

This adds a Fuchsia module to clang-tidy to warn for features that are 
disallowed. The following checks were added to the new module:

fuchsia-default-arguments: Check to prevent use of default arguments in 
declared or called functions.
fuchsia-overloaded-operator: Check to prevent operator overloading.
fuchsia-statically-constructed-objects: Check to prevent creation of  
statically-stored objects.
fuchsia-thread-local: Check to prevent thread-local storage.
fuchsia-trailing-return: Check to prevent functions with trailing returns.
fuchsia-virtual-inheritance: Check to prevent the definition of classes with 
virtual inheritance.
fuchsia-multiple-inheritance: Check to prevent multiple inheritance in Fuchsia.

Tests and documentation are also updated.


https://reviews.llvm.org/D40108

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/fuchsia/CMakeLists.txt
  clang-tidy/fuchsia/DefaultArgumentsCheck.cpp
  clang-tidy/fuchsia/DefaultArgumentsCheck.h
  clang-tidy/fuchsia/FuchsiaTidyModule.cpp
  clang-tidy/fuchsia/MultipleInheritanceCheck.cpp
  clang-tidy/fuchsia/MultipleInheritanceCheck.h
  clang-tidy/fuchsia/OverloadedOperatorCheck.cpp
  clang-tidy/fuchsia/OverloadedOperatorCheck.h
  clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
  clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.h
  clang-tidy/fuchsia/ThreadLocalCheck.cpp
  clang-tidy/fuchsia/ThreadLocalCheck.h
  clang-tidy/fuchsia/TrailingReturnCheck.cpp
  clang-tidy/fuchsia/TrailingReturnCheck.h
  clang-tidy/fuchsia/VirtualInheritanceCheck.cpp
  clang-tidy/fuchsia/VirtualInheritanceCheck.h
  clang-tidy/tool/CMakeLists.txt
  clang-tidy/tool/ClangTidyMain.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/fuchsia-default-arguments.rst
  docs/clang-tidy/checks/fuchsia-multiple-inheritance.rst
  docs/clang-tidy/checks/fuchsia-overloaded-operator.rst
  docs/clang-tidy/checks/fuchsia-statically-constructed-objects.rst
  docs/clang-tidy/checks/fuchsia-thread-local.rst
  docs/clang-tidy/checks/fuchsia-trailing-return.rst
  docs/clang-tidy/checks/fuchsia-virtual-inheritance.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/fuchsia-default-arguments.cpp
  test/clang-tidy/fuchsia-multiple-inheritance.cpp
  test/clang-tidy/fuchsia-overloaded-operator.cpp
  test/clang-tidy/fuchsia-statically-constructed-objects.cpp
  test/clang-tidy/fuchsia-thread-local.cpp
  test/clang-tidy/fuchsia-trailing-return.cpp
  test/clang-tidy/fuchsia-virtual-inheritance.cpp

Index: test/clang-tidy/fuchsia-virtual-inheritance.cpp
===
--- /dev/null
+++ test/clang-tidy/fuchsia-virtual-inheritance.cpp
@@ -0,0 +1,54 @@
+// RUN: %check_clang_tidy %s fuchsia-virtual-inheritance %t
+
+class A {
+public:
+  A(int value) : val(value) {}
+
+  int do_A() { return val; }
+
+private:
+  int val;
+};
+
+class B : public virtual A {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: [kernel-c++] virtual inheritance is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT: class B : public virtual A {
+public:
+  B() : A(0) {}
+  int do_B() { return 1 + do_A(); }
+};
+
+class C : public virtual A {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: [kernel-c++] virtual inheritance is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT: class C : public virtual A {
+public:
+  C() : A(0) {}
+  int do_C() { return 2 + do_A(); }
+};
+
+class D : public B, public C {
+  // CHECK-MESSAGES: [[@LINE-1]]:1: warning: [kernel-c++] virtual inheritance is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT: class C : public B, public C {
+public:
+  D(int value) : A(value), B(), C() {}
+  // CHECK-MESSAGES: [[@LINE-1]]:28: warning: [kernel-c++] constructing a class which inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  D(int value) : A(value), B(), C() {}
+  // CHECK-MESSAGES: [[@LINE-3]]:33: warning: [kernel-c++] constructing a class which inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  D(int value) : A(value), B(), C() {}
+
+  int do_D() { return do_A() + do_B() + do_C(); }
+};
+
+int main(void) {
+  A *a = new A(0);
+  B *b = new B();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: [kernel-c++] constructing a class which inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  B *b = new B();
+  C *c = new C();
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: [kernel-c++] constructing a class which inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  C *c = new C();
+  D *d = new D(0);
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: [kernel-c++] constructing a class which inherits a virtual base class is disallowed [fuchsia-virtual-inheritance]
+  // CHECK-NEXT:  D *d = new D(0);
+  return 0;
+}
Index: