[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:38-40
+Message = "'extern' keyword has no effect";
+  } else {
+Message = "redundant 'extern' keyword";

These messages alone will be quite confusing unless the user has enough 
context. Please expand the messages with the explanations.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:48-50
+  StringRef Text =
+  Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
+   *Result.SourceManager, getLangOpts());

I suspect there are many ways the `extern` substring can appear in a function 
definition (`void my_extern()`, `[[some_attribute("extern")]] void f()`, `void 
/*extern*/ f()`, etc.). I don't immediately see how these possible false 
positives are handled here. Am I missing something obvious?

Is it more robust to re-lex the range and search for the raw identifier token 
with the text `extern`?


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D33841#1589212 , @koldaniel wrote:

> Hi, do you have any additional comments?


Could you mark "Done" the comments you've addressed?


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel added a comment.

Hi, do you have any additional comments?


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel updated this revision to Diff 206190.

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

https://reviews.llvm.org/D33841

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/RedundantExternCheck.cpp
  clang-tidy/readability/RedundantExternCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/readability-redundant-extern.rst
  test/clang-tidy/readability-redundant-extern.cpp

Index: test/clang-tidy/readability-redundant-extern.cpp
===
--- /dev/null
+++ test/clang-tidy/readability-redundant-extern.cpp
@@ -0,0 +1,74 @@
+// RUN: %check_clang_tidy %s readability-redundant-extern %t
+
+extern int f();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: int f();
+
+int extern f() { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: int f() { return 0; }
+
+extern "C" int g();
+// CHECK-FIXES: extern "C" int g();
+
+extern "C" int g() { return 0; }
+// CHECK-FIXES: extern "C" int g() { return 0; }
+
+extern "C++" int h();
+// CHECK-FIXES: extern "C++" int h();
+
+extern "C++" int h() { return 0; }
+// CHECK-FIXES: extern "C++" int h() { return 0; }
+
+inline extern void foo_inline();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: inline void foo_inline();
+
+#define FOO_EXTERN extern
+FOO_EXTERN void foo_macro_1();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_EXTERN void foo_macro_1();
+
+#define FOO_INLINE inline
+FOO_INLINE extern void foo_macro_2();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_INLINE extern void foo_macro_2();
+
+#define FOO_EXTERN_INLINE inline extern
+FOO_EXTERN_INLINE void foo_macro_3();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: FOO_EXTERN_INLINE void foo_macro_3();
+
+void file_scope();
+// CHECK-FIXES: void file_scope();
+
+void another_file_scope(int _extern);
+// CHECK-FIXES: void another_file_scope(int _extern);
+
+namespace {
+extern void namespace_1();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: 'extern' keyword has no effect [readability-redundant-extern]
+// CHECK-FIXES: void namespace_1();
+}
+
+namespace a {
+namespace {
+namespace b {
+extern void namespace_2();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: 'extern' keyword has no effect [readability-redundant-extern]
+// CHECK-FIXES: void namespace_2();
+}
+}
+}
+
+namespace {
+extern "C" void namespace_3();
+// CHECK-FIXES: extern "C" void namespace_3();
+}
+
+#define FOO_EXTERN extern
+typedef int extern_int;
+
+extern_int FOO_EXTERN typedef_extern_foo();
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: redundant 'extern' keyword [readability-redundant-extern]
+// CHECK-FIXES: extern_int FOO_EXTERN typedef_extern_foo();
Index: docs/clang-tidy/checks/readability-redundant-extern.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/readability-redundant-extern.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - readability-redundant-extern
+
+readability-redundant-extern
+
+
+Removes the redundant or unnecessary ``extern`` keywords from code.
+
+``extern`` is redundant in function declarations, and has no effect in
+namespaces
+
+The default language linkage is C++, so without any additional parameters
+it is redundant (extern "C++" can also be redundant, but it depends on
+the context). In C context (extern "C") the situation is the same, extern
+keyword is redundant for function declarations
+
+.. code-block:: c++
+
+  extern void h();
+
+  namespace {
+extern void namespace_1();  
+  }
+
+  #define FOO_EXTERN extern
+  FOO_EXTERN void foo_macro_1();
+
+  #define FOO_EXTERN extern
+  typedef int extern_int;
+  extern_int FOO_EXTERN typedef_extern_foo();
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -240,6 +240,11 @@
   but either don't specify it or the clause is specified but with the kind
   other than ``none``, and suggests to use the ``default(none)`` clause.
 
+- New :doc:`readability-redundant-extern
+  ` check.
+
+  Removes the redundant ``extern`` keywords.
+
 Improvements to clang-include-fixer
 ---
 
Index: clang-tidy/readability/RedundantExternCheck.h
===
--- /dev/null
+++ clang-tidy/readability/RedundantExternCheck.h
@@ -0,0 +1,34 @@
+//===--- RedundantExternCheck.h - clang-tidy *- C++ -*-===//
+//
+// 

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel marked an inline comment as done.
koldaniel added inline comments.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:43-44
+
+  if (FD->getBeginLoc().isMacroID())
+return;
+

lebedev.ri wrote:
> koldaniel wrote:
> > lebedev.ri wrote:
> > > Similarly, do this in `registerMatchers()`
> > Could you please help me how could I achieve that? I did not find any 
> > matchers which match for macros.
> You can simply add your own matchers, see e.g. `AST_MATCHER()`'s in  
> `AvoidCArraysCheck.cpp`.
I see, I can update the checker based on your previous comment (move  `if 
(FD->getStorageClass() != SC_Extern)` to `registerMatchers()`), but moving the 
macro check would disable the warning. We do not want to ignore the macro 
findings, we just don't want to apply fix-its.


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:43-44
+
+  if (FD->getBeginLoc().isMacroID())
+return;
+

koldaniel wrote:
> lebedev.ri wrote:
> > Similarly, do this in `registerMatchers()`
> Could you please help me how could I achieve that? I did not find any 
> matchers which match for macros.
You can simply add your own matchers, see e.g. `AST_MATCHER()`'s in  
`AvoidCArraysCheck.cpp`.


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Daniel Kolozsvari via Phabricator via cfe-commits]

koldaniel marked an inline comment as done.
koldaniel added inline comments.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:30
+
+  if (FD->getStorageClass() != SC_Extern)
+return;

lebedev.ri wrote:
> Can you do that in `registerMatchers()`?
The only way I found for that is to use the matcher `hasExternalFormalLinkage`, 
but the problem with it is that now the checker warns not only for the 
redundant `extern` usage, but for the unnecessary too - in the latter case the 
linkage is internal, and I think by checking the storage class we can determine 
whether the `extern` keyword has been used or not.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:43-44
+
+  if (FD->getBeginLoc().isMacroID())
+return;
+

lebedev.ri wrote:
> Similarly, do this in `registerMatchers()`
Could you please help me how could I achieve that? I did not find any matchers 
which match for macros.


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

https://reviews.llvm.org/D33841



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

[PATCH] D33841: [clang-tidy] redundant 'extern' keyword check

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:30
+
+  if (FD->getStorageClass() != SC_Extern)
+return;

Can you do that in `registerMatchers()`?



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:43-44
+
+  if (FD->getBeginLoc().isMacroID())
+return;
+

Similarly, do this in `registerMatchers()`



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:49
+   *Result.SourceManager, getLangOpts());
+
+  int Offset = Text.find("extern");

`StringLiteral Extern = StringLiteral("extern");`



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:50
+
+  int Offset = Text.find("extern");
+

Extern



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:53
+  SourceRange ExternRange(BeginLoc.getLocWithOffset(Offset),
+  BeginLoc.getLocWithOffset(Offset + 
strlen("extern")));
+

Extern.size()



Comment at: clang-tidy/readability/RedundantExternCheck.cpp:59
+
+  if (!ExternText.equals("extern")) // Final check: typedefs, etc.
+return;

Extern



Comment at: docs/clang-tidy/checks/readability-redundant-extern.rst:10
+
+The default language linkage is C++, so without any additional parameters it 
is redundant (extern "C++" can also be redundant, but it depends on the 
context). In C context (extern "C") the situation is the same, extern keyword 
is redundant for function declarations
+

Please 80-char wrap



Comment at: docs/clang-tidy/checks/readability-redundant-extern.rst:14
+
+  extern void h();
+

More examples? A namespace one?


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

https://reviews.llvm.org/D33841



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