[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:67
+
+  diag(MD->getLocation(), DiagnosticMessage);
+}

And once again, i'm going to bitch about useless diagnostic messages:
```
[1/357] Building CXX object 
src/CMakeFiles/rawspeed.dir/librawspeed/metadata/CameraMetaData.cpp.o
FAILED: src/CMakeFiles/rawspeed.dir/librawspeed/metadata/CameraMetaData.cpp.o 
/usr/bin/cmake -E __run_co_compile --tidy=/usr/local/bin/clang-tidy 
--source=../src/librawspeed/metadata/CameraMetaData.cpp -- 
/usr/local/bin/clang++  -DDEBUG -D_GLIBCXX_SANITIZE_VECTOR -Isrc 
-I../src/librawspeed -isystem ../src/external -std=c++14 -Wall -Wextra 
-Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-conversion 
-Wno-covered-switch-default -Wno-deprecated -Wno-double-promotion 
-Wno-exit-time-destructors -Wno-global-constructors 
-Wno-gnu-zero-variadic-macro-arguments -Wno-old-style-cast -Wno-padded 
-Wno-switch-enum -Wno-unused-macros -Wno-unused-parameter -Wno-weak-vtables 
-Wno-zero-as-null-pointer-constant -Wextra-semi -O3 -fno-optimize-sibling-calls 
-fsanitize=address -fno-omit-frame-pointer -fno-common -U_FORTIFY_SOURCE 
-fsanitize-address-use-after-scope -fsanitize=undefined 
-fno-sanitize-recover=undefined -fsanitize=integer 
-fno-sanitize-recover=integer -fPIC   -march=native -g3 -ggdb3 -Werror -pthread 
-fopenmp=libomp -std=c++14 -MD -MT 
src/CMakeFiles/rawspeed.dir/librawspeed/metadata/CameraMetaData.cpp.o -MF 
src/CMakeFiles/rawspeed.dir/librawspeed/metadata/CameraMetaData.cpp.o.d -o 
src/CMakeFiles/rawspeed.dir/librawspeed/metadata/CameraMetaData.cpp.o -c 
../src/librawspeed/metadata/CameraMetaData.cpp
error: macro used to declare a constant; consider using a 'constexpr' constant 
[cppcoreguidelines-macro-usage,-warnings-as-errors]
error: macro used to declare a constant; consider using a 'constexpr' constant 
[cppcoreguidelines-macro-usage,-warnings-as-errors]
9174 warnings generated.
Suppressed 9172 warnings (9172 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use 
-system-headers to display errors from system headers as well.
2 warnings treated as errors
ninja: build stopped: subcommand failed.
```
1. It complains about something that has to location (so it was declared in 
command line?)
2. I have absolutely no clue whatsoever on what macro it complains.
   And i don't really see a way to find it out, without modifying clang-tidy :/


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344940: [clang-tidy] implement cppcoreguidelines macro 
rules (authored by JonasToth, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D41648?vs=170472=170473#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- clang-tidy/cppcoreguidelines/MacroUsageCheck.h
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,48 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+#include 
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context),
+AllowedRegexp(Options.get("AllowedRegexp", "^DEBUG_*")),
+CheckCapsOnly(Options.get("CheckCapsOnly", 0)) {}
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+  void warnNaming(const MacroDirective *MD);
+
+private:
+  /// A regular expression that defines how allowed macros must look like.
+  std::string AllowedRegexp;
+  /// Control if only the check shall only test on CAPS_ONLY macros.
+  bool CheckCapsOnly;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
Index: clang-tidy/cppcoreguidelines/CMakeLists.txt
===
--- clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -4,6 +4,7 @@
   AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
+  MacroUsageCheck.cpp
   NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -0,0 +1,95 @@
+//===--- MacroUsageCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "MacroUsageCheck.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/Regex.h"
+#include 
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+namespace {
+
+bool isCapsOnly(StringRef Name) {
+  return std::all_of(Name.begin(), Name.end(), [](const char c) {
+if (std::isupper(c) || std::isdigit(c) || c == '_')
+  return true;
+return false;
+  });
+}
+
+class MacroUsageCallbacks : public PPCallbacks {
+public:
+  MacroUsageCallbacks(MacroUsageCheck *Check, StringRef RegExp, bool CapsOnly)
+  : Check(Check), RegExp(RegExp), CheckCapsOnly(CapsOnly) {}
+  void MacroDefined(const Token ,
+const MacroDirective *MD) override {
+if (MD->getMacroInfo()->isUsedForHeaderGuard() ||
+MD->getMacroInfo()->getNumTokens() == 0)
+  return;
+
+StringRef MacroName = MacroNameTok.getIdentifierInfo()->getName();
+if (!CheckCapsOnly && !llvm::Regex(RegExp).match(MacroName))
+  Check->warnMacro(MD);
+
+if (CheckCapsOnly && 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 170472.
JonasToth added a comment.

- [Doc] add missing release notes


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -89,6 +89,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 170471.
JonasToth added a comment.

- add missing private


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -89,6 +89,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-non-private-member-variables-in-classes (redirects to 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 170470.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.

- address review nits


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -89,6 +89,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:34-35
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+  void warnNaming(const MacroDirective *MD);
+

aaron.ballman wrote:
> Nit: these can be private functions instead.
They are used by the PPCallbacks. I think adding a `friend` is not worth it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[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 some minor nits.




Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:71
+void MacroUsageCheck::warnMacro(const MacroDirective *MD) {
+  auto MessageFactory = [](const MacroInfo *Info) {
+/// A variadic macro is function-like at the same time. Therefore variadic

It seems unnecessarily complicated, to me, to use a lambda here. Why not use a 
StringRef local variable?
```
StringRef Msg = "blah";
if (Info->isVariadic())
  Msg = "blah";
else if (Info->isFunctionLike())
  Msg = "blah";
```



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78
+ "variadic template function";
+else if (Info->isFunctionLike())
+  return "function-like macro used; consider a 'constexpr' template "

Nit: `else` after `return`



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:81
+ "function";
+else
+  return "macro used to declare a constant; consider using a 'constexpr' "

Nit: same



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:34-35
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+  void warnNaming(const MacroDirective *MD);
+

Nit: these can be private functions instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 169410.
JonasToth added a comment.

- make the logic with variadic clear


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 169403.
JonasToth added a comment.

- remove unused enum in header file, no idea what i intended to do with it :D


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 169402.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- add tests and adjust doc
- one more test case


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,18 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define PROBLEMATIC_VARIADIC2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+#define TEST_VARIADIC2(x, ...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+//
+#define problematic_variadic2(x, ...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Updated




Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78
+"function-like macro used; consider a 'constexpr' template function";
+  if (Info->isVariadic())
+DiagnosticMessage = "variadic macro used; consider using a 'constexpr' "

aaron.ballman wrote:
> Aren't all vararg macros also function-like macros, so this will trigger two 
> diagnostics for all variadic macro definitions?
`MacroInfo` contains bits about function-like and beeing variadic separatly 
(gnu variadic as well, but thats included in `isVariadic`). There are no double 
warnings created.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41648#1253647, @JonasToth wrote:

> @aaron.ballman What do you think of the current version of this check?
>  As migration strategy the option for `CAPS_ONLY` is provided, otherwise the 
> filtering is provided to silence necessary macros (which kind of enforces a 
> common prefix for macros). This does implement both the cppcoreguidelines and 
> allows migration.


I think that's a reasonable approach.




Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:78
+"function-like macro used; consider a 'constexpr' template function";
+  if (Info->isVariadic())
+DiagnosticMessage = "variadic macro used; consider using a 'constexpr' "

Aren't all vararg macros also function-like macros, so this will trigger two 
diagnostics for all variadic macro definitions?



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:26
+
+Boolean flag to warn all macros except those with CAPS_ONLY names.
+This option is intended to ease introduction of this check into older

warn all macros -> warn on all macros



Comment at: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp:14
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using 
a 'constexpr' variadic template function

Please add a test like:
```
#define PROBLEMATIC_VARIADIC_TWO(x, ...) (__VA_ARGS__)
```
To ensure that you don't get two diagnostics (one for function-like and one for 
variadic).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@aaron.ballman What do you think of the current version of this check?
As migration strategy the option for `CAPS_ONLY` is provided, otherwise the 
filtering is provided to silence necessary macros (which kind of enforces a 
common prefix for macros). This does implement both the cppcoreguidelines and 
allows migration.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 168095.
JonasToth added a comment.

- add more positive tests
- Merge branch 'master' into check_macros_usage


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,21 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define OKISH_CONSTANT 42
+#define OKISH_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define OKISH_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 168094.
JonasToth added a comment.

- Merge branch 'master' into check_macros_usage


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 164948.
JonasToth marked 7 inline comments as done.
JonasToth added a comment.

- Merge branch 'master' into check_macros_usage
- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function-like macro used; consider a 'constexpr' template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a 'constexpr' variadic template function
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro definition does not define the macro name using all uppercase characters
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -88,6 +88,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 162450.
JonasToth added a comment.

- Merge branch 'master' into check_macros_usage


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -84,6 +84,7 @@
cppcoreguidelines-avoid-magic-numbers (redirects to readability-magic-numbers) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:77
+DiagnosticMessage =
+"function like macro used; consider a (constexpr) template function";
+  if (Info->isVariadic())

function like -> function-like

Why is constexpr in parentheses?



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:80
+DiagnosticMessage =
+"variadic macro used; consider using a variadic template";
+

Should this also suggest a constexpr variadic template?



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:86
+void MacroUsageCheck::warnNaming(const MacroDirective *MD) {
+  diag(MD->getLocation(), "use CAPS_ONLY for macros");
+}

How about: `macro definition does not define the macro name using all uppercase 
characters` or something along those lines?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

>> OpenCV isn't clean either, here the results:
>> 
>> Filter: 
>> `ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*`
> 
> This one worries me a bit more because of patterns like `cl*` and `calc*` -- 
> those seem like they're not uncommon and the pattern may silence otherwise 
> valid diagnostics.

The macros are worriesome. Instead of using proper function overloading or 
similar constructs, they defined macros that replace function names + 
arguments. I would say, these macros were laziness, because C++ provides the 
proper language tools to deal with the missing overloading capabilities for C 
functions. I removed them because some many macros did exist.

>> - it is possible to reduce macro usage to a minimal amount, and the complex 
>> macros like AST stuff can be filtered with the regex. Furthermore, 
>> restricting all macros to a "macro namespace" is possible for sure.
> 
> I'm not certain I understand what you mean by "macro namespace". Can you 
> clarify?

With "macro namespace" i mean a common prefix for macros, like 
`LLVM_UNREACHABLE`, `DEBUG_..`.

> I'm still a bit worried about using regex to filter the results. It seems 
> like any real world project is going to require somewhere between a 
> reasonable and an unreasonable amount of configuration to reduce the noise. 
> Perhaps as a first pass, however, it will suffice. Hopefully we can add other 
> heuristics to reduce the false positives as we see patterns emerge from real 
> world usage.

It was hard to find reasonable patterns. I definitely saw the usage that is not 
supposed to happen, like simulating inline functions, overloading, ...

>> Things I would like to add to the check:
>> 
>> - my little filtering script is valuable for developers, that want to 
>> address the macro issue. It should be added to the docs and everyone can 
>> make something based on that. It will be linux centered.
> 
> Why does the usage need to be limited to Linux?

I created small `sed` scripts with a chain of `uniq` and `sort`, so it will be 
UNIX, but windows falls short i guess.

>> - enforcing ALL_CAPS, including its usage
> 
> What does "including its usage" mean?

The usage of the macro in code. That means not only definition of the macro is 
replaced, but all occurences. This can only be done if *all* macros are treated 
the same. Otherwise different TU, different transformation.

>> Code transformation has the problems of scope and potentially breaking code 
>> badly, because clang-tidy wasn't run over all of the code.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41648#1047799, @JonasToth wrote:

> I checked several codebases and implemented a little helper script to see 
> which kind of macros exist. Then I added some filter regex as configuration 
> and tried again, here are the results:
>
> For https://github.com/nlohmann/json which is a very high quality codebase, 
> the results were as expected: very clean, and good macro usage: (except a 
> `#define private public` for tests)
>
> Filter: `JSON*|NLOHMANN*`
>  F5913499: all_macros.txt 
>
> F5913498: filtered_macros.txt 
>
> CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for 
> all macros.
>
> Filter:  
> `_FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*`


This is a pretty long list of macro patterns to have to filter, but doesn't 
look to problematic.

> F5913502: warned_macros.txt 
> 
> F5913501: filtered_macros.txt 
> 
> OpenCV isn't clean either, here the results:
> 
> Filter: 
> `ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*`

This one worries me a bit more because of patterns like `cl*` and `calc*` -- 
those seem like they're not uncommon and the pattern may silence otherwise 
valid diagnostics.

> F5913504: all_macros.txt 
> 
> F5913503: filtered_modules_macros.txt 
> 
> libtorrent https://github.com/arvidn/libtorrent has reasonable macro usage
> 
> Filter: `DEBUG*|LIBTORRENT*|TORRENT*|UNI*`
> 
> F5913519: filtered_macros.txt 
> 
> F5913518: all_macros.txt 
> 
> LLVM lib/ defines many macros, almost all of them are ALL_CAPS
> 
> Filter: 
> `AARCH64*|X86*|ARM*|AMDGPU*|ASAN*|ATTR*|CHECK*|COMMON*|CV*|CXX*|DEBUG*|DEC*|DEFINE*|ESAN*|ENUM*|FMA*|FUZZER*|HANDLE*|INTERCEPTOR*|LSAN*|MATH*|MSAN*|OPENMP*|TSAN*|TYPE*|UBSAN*`
> 
> F5913505: all_lib_macros.txt 
>  F5913528: filtered_lib_macros.txt 
> 
> LLVM tools/ is a similar story
>  Filter: 
> `ANALYSIS*|ASSERT*|CHECK*|DEBUG*|DECL*|DEPENDENT*|DIAG*|END*|ENUM*|GENERIC*|IMAGE*|KEYWORD*|LANG*|LLVM*|NON*|OBJC*|OPENMP*|OVERLOAD*|PROC*|REGISTER*|SANITIZER*|STMT*|TYPE*|X86*`
> 
> F5913566: all_tools_macros.txt 
> 
> F5913565: filtered_tools_macros.txt 
> 
> @aaron.ballman Would you like to see other projects checked? I think this is 
> a starting point for now.

I think this is a good starting point. It's certainly demonstrated that even 
clean projects cannot conform to the C++ Core Guidelines.

> My opinion based on what i see is
> 
> - maybe two modes for this check make sense, having one requiring CAPS_ONLY 
> and the currently implemented stricter check. This allows easier migration to 
> safer macro usage

I think I agree.

> - it is possible to reduce macro usage to a minimal amount, and the complex 
> macros like AST stuff can be filtered with the regex. Furthermore, 
> restricting all macros to a "macro namespace" is possible for sure.

I'm not certain I understand what you mean by "macro namespace". Can you 
clarify?

I'm still a bit worried about using regex to filter the results. It seems like 
any real world project is going to require somewhere between a reasonable and 
an unreasonable amount of configuration to reduce the noise. Perhaps as a first 
pass, however, it will suffice. Hopefully we can add other heuristics to reduce 
the false positives as we see patterns emerge from real world usage.

> Things I would like to add to the check:
> 
> - my little filtering script is valuable for developers, that want to address 
> the macro issue. It should be added to the docs and everyone can make 
> something based on that. It will be linux centered.

Why does the usage need to be limited to Linux?

> - enforcing ALL_CAPS, including its usage

What does "including its usage" mean?

> - (adding a prefix to all macro, passing the filter, including its usage)
> 
>   Code transformation has the problems of scope and potentially breaking code 
> badly, because clang-tidy wasn't run over all of the code.
> 
> The check is chatty, as expected, because macros in header files, especially 
> central ones, pollute everything. That is the reason for the check, too. 
>  Overall I am still in favor of this approach, seeing some obscure macros 
> that should be replaced with actual language features (like overloading, 
> .inline functions, constants, ...) in the checked codebases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41648#1047799, @JonasToth wrote:

> I checked several codebases and implemented a little helper script to see 
> which kind of macros exist. Then I added some filter regex as configuration 
> and tried again, here are the results:
>
> For https://github.com/nlohmann/json which is a very high quality codebase, 
> the results were as expected: very clean, and good macro usage: (except a 
> `#define private public` for tests)
>
> Filter: `JSON*|NLOHMANN*`
>  F5913499: all_macros.txt 
>
> F5913498: filtered_macros.txt 
>
> CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for 
> all macros.
>
> Filter:  
> `_FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*`
>
> F5913502: warned_macros.txt 
>
> F5913501: filtered_macros.txt 
>
> OpenCV isn't clean either, here the results:
>
> Filter: 
> `ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*`
>
> F5913504: all_macros.txt 
>
> F5913503: filtered_modules_macros.txt 
>
> libtorrent https://github.com/arvidn/libtorrent has reasonable macro usage
>
> Filter: `DEBUG*|LIBTORRENT*|TORRENT*|UNI*`
>
> F5913519: filtered_macros.txt 
>
> F5913518: all_macros.txt 
>
> LLVM lib/ defines many macros, almost all of them are ALL_CAPS
>
> Filter: 
> `AARCH64*|X86*|ARM*|AMDGPU*|ASAN*|ATTR*|CHECK*|COMMON*|CV*|CXX*|DEBUG*|DEC*|DEFINE*|ESAN*|ENUM*|FMA*|FUZZER*|HANDLE*|INTERCEPTOR*|LSAN*|MATH*|MSAN*|OPENMP*|TSAN*|TYPE*|UBSAN*`
>
> F5913505: all_lib_macros.txt 
>  F5913528: filtered_lib_macros.txt 
>
> LLVM tools/ is a similar story
>  Filter: 
> `ANALYSIS*|ASSERT*|CHECK*|DEBUG*|DECL*|DEPENDENT*|DIAG*|END*|ENUM*|GENERIC*|IMAGE*|KEYWORD*|LANG*|LLVM*|NON*|OBJC*|OPENMP*|OVERLOAD*|PROC*|REGISTER*|SANITIZER*|STMT*|TYPE*|X86*`
>
> F5913566: all_tools_macros.txt 
>
> F5913565: filtered_tools_macros.txt 
>
> @aaron.ballman Would you like to see other projects checked? I think this is 
> a starting point for now.
>
> My opinion based on what i see is
>
> - maybe two modes for this check make sense, having one requiring CAPS_ONLY 
> and the currently implemented stricter check. This allows easier migration to 
> safer macro usage
> - it is possible to reduce macro usage to a minimal amount, and the complex 
> macros like AST stuff can be filtered with the regex. Furthermore, 
> restricting all macros to a "macro namespace" is possible for sure.
>
>   Things I would like to add to the check:
> - my little filtering script is valuable for developers, that want to address 
> the macro issue. It should be added to the docs and everyone can make 
> something based on that. It will be linux centered.
> - enforcing ALL_CAPS, including its usage
> - (adding a prefix to all macro, passing the filter, including its usage)
>
>   Code transformation has the problems of scope and potentially breaking code 
> badly, because clang-tidy wasn't run over all of the code.
>
> The check is chatty, as expected, because macros in header files, especially 
> central ones, pollute everything. That is the reason for the check, too. 
>  Overall I am still in favor of this approach, seeing some obscure macros 
> that should be replaced with actual language features (like overloading, 
> .inline functions, constants, ...) in the checked codebases.


@aaron.ballman Did you have time to look at my analysis result?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24
+
+.. option:: CheckCapsOnly
+

Eugene.Zelenko wrote:
> Will be good idea to generalize this option with other check which deal with 
> macros.
I dont think that the bugprone- macro checks would benefit from such an option. 
They all address specific issues that can occur with macros and that occur even 
if you have restrained usage of macros.

The general goal of this check is to remove macros for pretty much everything. 
The Regex and CapsOnly option only try to lower the bar for enabling this check 
and allow gradual adoption.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D41648#1048312, @JonasToth wrote:

> Which checks do you have in mind?


bugprone-macro-*, bugprone-multiple-statement-macro.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Which checks do you have in mind?

Am 26.03.2018 um 18:07 schrieb Eugene Zelenko via Phabricator:

> Eugene.Zelenko added inline comments.
> 
> 
>  Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24
>  +
>  +.. option:: CheckCapsOnly
>  +
> 
>  
> 
> Will be good idea to generalize this option with other check which deal with 
> macros.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D41648


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:24
+
+.. option:: CheckCapsOnly
+

Will be good idea to generalize this option with other check which deal with 
macros.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:26
+
+This boolean flag disables all warnings for macros and checks only that 
+macro names are CAPS_ONLY. This option is meant to ease introduction of 
this

Will be good idea to rephrase statement: //except those with CAPS_ONLY names//?



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:27
+This boolean flag disables all warnings for macros and checks only that 
+macro names are CAPS_ONLY. This option is meant to ease introduction of 
this
+check into older code bases. Default value is `0`/`false`.

is //intended//?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 139793.
JonasToth added a comment.

- [Feature] implement CAPS_ONLY features, adjust docs


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-caps-only.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.CheckCapsOnly, value: 1}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define problematic_constant 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_function(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#define problematic_variadic(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: use CAPS_ONLY for macros
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -75,6 +75,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I checked several codebases and implemented a little helper script to see which 
kind of macros exist. Then I added some filter regex as configuration and tried 
again, here are the results:

For https://github.com/nlohmann/json which is a very high quality codebase, the 
results were as expected: very clean, and good macro usage: (except a `#define 
private public` for tests)

Filter: `JSON*|NLOHMANN*`
F5913499: all_macros.txt 

F5913498: filtered_macros.txt 

CMake on the other hand uses many macros and don't obey CAPS_ONLY rules for all 
macros.

Filter:  
`_FILE_OFFSET_BITS|AE_*|ARCHIVE_*|cal_*|CMAKE_*|cm*|CM_*|CPP_*|CURL*|E_*|exp_*|F90*|jp*|JSON_*|KW*|kw*|O_*|REQ_*|UV_*|YY*|yy*|Z_*`

F5913502: warned_macros.txt 

F5913501: filtered_macros.txt 

OpenCV isn't clean either, here the results:

Filter: 
`ASSERT*|ALL*|CAL*|CC*|CALC*|calc*|CL*|cl*|CUDA*|CV*|cv*|EXPECT*|GTEST*|FUNCTOR*|HAVE*|ICV*|IPL*|IPP*|ipp*|__itt*|ITT*|JAS*|jas*|MESSAGE*|MAX*|OCL*|opengl*|OPENCV*|TYP*`

F5913504: all_macros.txt 

F5913503: filtered_modules_macros.txt 

libtorrent https://github.com/arvidn/libtorrent has reasonable macro usage

Filter: `DEBUG*|LIBTORRENT*|TORRENT*|UNI*`

F5913519: filtered_macros.txt 

F5913518: all_macros.txt 

LLVM lib/ defines many macros, almost all of them are ALL_CAPS

Filter: 
`AARCH64*|X86*|ARM*|AMDGPU*|ASAN*|ATTR*|CHECK*|COMMON*|CV*|CXX*|DEBUG*|DEC*|DEFINE*|ESAN*|ENUM*|FMA*|FUZZER*|HANDLE*|INTERCEPTOR*|LSAN*|MATH*|MSAN*|OPENMP*|TSAN*|TYPE*|UBSAN*`

F5913505: all_lib_macros.txt 
F5913528: filtered_lib_macros.txt 

LLVM tools/ is a similar story
Filter: 
`ANALYSIS*|ASSERT*|CHECK*|DEBUG*|DECL*|DEPENDENT*|DIAG*|END*|ENUM*|GENERIC*|IMAGE*|KEYWORD*|LANG*|LLVM*|NON*|OBJC*|OPENMP*|OVERLOAD*|PROC*|REGISTER*|SANITIZER*|STMT*|TYPE*|X86*`

F5913566: all_tools_macros.txt 

F5913565: filtered_tools_macros.txt 

@aaron.ballman Would you like to see other projects checked? I think this is a 
starting point for now.

My opinion based on what i see is

- maybe two modes for this check make sense, having one requiring CAPS_ONLY and 
the currently implemented stricter check. This allows easier migration to safer 
macro usage
- it is possible to reduce macro usage to a minimal amount, and the complex 
macros like AST stuff can be filtered with the regex. Furthermore, restricting 
all macros to a "macro namespace" is possible for sure.

Things I would like to add to the check:

- my little filtering script is valuable for developers, that want to address 
the macro issue. It should be added to the docs and everyone can make something 
based on that. It will be linux centered.
- enforcing ALL_CAPS, including its usage
- (adding a prefix to all macro, passing the filter, including its usage)

Code transformation has the problems of scope and potentially breaking code 
badly, because clang-tidy wasn't run over all of the code.

The check is chatty, as expected, because macros in header files, especially 
central ones, pollute everything. That is the reason for the check, too. 
Overall I am still in favor of this approach, seeing some obscure macros that 
should be replaced with actual language features (like overloading, .inline 
functions, constants, ...) in the checked codebases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 139707.
JonasToth added a comment.

- Merge branch 'master' into check_macros_usage
- [Doc] update to new doc style


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -75,6 +75,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -82,6 +82,12 @@
   with looping constructs. Every backward jump is rejected. Forward jumps are
   only allowed in nested loops.
 
+- New :doc:`cppcoreguidelines-macro-usage
+  ` check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New :doc:`fuchsia-multiple-inheritance
   ` check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,45 @@
+//===--- 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41648#976665, @aaron.ballman wrote:

> My gut reaction is still that this check is going to be too chatty on real 
> world code bases to be of value beyond compliance with the C++ Core 
> Guidelines, but that's also sufficient enough reason to have the check in the 
> first place. Perhaps a reasonable approach would be to put in the heuristics 
> you think make the most sense, then run the check over some large real world 
> C++ code bases to see how many diagnostics are generated. That will also help 
> you to see code patterns to modify your heuristic until you're happy with the 
> results. Then we can debate whether there's more left to do from the data.


Agreed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

My gut reaction is still that this check is going to be too chatty on real 
world code bases to be of value beyond compliance with the C++ Core Guidelines, 
but that's also sufficient enough reason to have the check in the first place. 
Perhaps a reasonable approach would be to put in the heuristics you think make 
the most sense, then run the check over some large real world C++ code bases to 
see how many diagnostics are generated. That will also help you to see code 
patterns to modify your heuristic until you're happy with the results. Then we 
can debate whether there's more left to do from the data.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Using regex could be a reasonable way out, but isn't going to solve the 
> problem in general because not all of the macro names are ones the user has 
> control over. Having to whitelist dozens of macros by name means this check 
> is simply going to be disabled by the user as being low-value.

Yes. I think with the regex its exactly implemented like the rules say. But 
adding sugar is still reasonable. I will run the check over llvm to get a 
feeling how much is still left after silence `LLVM_*` and others that are 
clearly scoped.

> Except that's not the only form of conditional compilation I'm worried about. 
> See Compiler.h where we do things like `#define __has_attribute(x) 0`, which 
> is a perfectly reasonable macro to define (and is an example of a macro name 
> outside of the user's control, so they cannot simply prefix it for a 
> whitelist).

Having a regex allows to add `__has_attribute` to it or a pattern like 
`__has*`. But how many cases for such macros are left? The goal of the 
guidelines is to change coding style and requiring to silence _SOME_ warnings 
is reasonable to me.

> However, I have no idea how we would handle cases like LLVM_LIKELY and 
> LLVM_UNLIKELY, but expecting users to NOLINT 500+ lines of common macro code 
> doesn't seem like a good first approach. I'd be curious to know what the C++ 
> Core Guidelines folks think about those use cases and how they would 
> recommend rewriting the code to adhere to their guidelines. Do they really 
> expect users to use *no* macros in C++ code outside of header include guards 
> and noop definitions even when asked about reasonable use cases like 
> cross-compiler attributes or builtins? I mean, many of these things cannot 
> even use the attribute the C++ Core Guidelines require for silencing such 
> diagnostics -- how do they want to use gsl::suppress to silence a diagnostic 
> on a macro definition?

I think the long term goal is removing the preprocessor, but especially 
compiler specifics are very unlikely to get away soon. 
They themself make exceptions: ES.33: If you must use macros, give them unique 
names 


>> 2. Compiler intrinsics These are similar to attributes and could maybe even 
>> treated the same.
> 
> I'm less certain about this one -- with attributes, there's syntax that we 
> can inspect (is there an `__attribute__` or `__declspec` keyword?), but with 
> intrinsics there's no commonality. Or are you thinking of tablegenning the 
> list of intrinsics?

I thought mostly of `__builtin` when proposing. Checking the MSVC docs showed 
that not all compilers do follow such nice naming conventions. Adding a clang 
and gcc list of builtins might still be manageable with basic string handling.
I feel that keeping any list of all compiler intrinsics is too much.

>> 3. Compatibility with older C++ Standards Stuff like `#define OVERRIDE 
>> override` if the code is compiled with C++11 or newer might be acceptable 
>> for stepwise modernization. This can can be considered as program text 
>> manipulation that was explicitly forbidden but does not obscure code so an 
>> exception could be made. Such macros can be detected with string comparisons.
> 
> How would you generalize this? I've seen people use macros for arbitrary 
> keywords (`#define CONST const` and `#define false false`) as well as macros 
> for arbitrary syntax (`#define DEFAULTED {}` or `#define DEFAULTED = 
> default`).

Not sure about all. I think new keywords like `#define OVERRIDE override` 
should definitly be allowed. But things like `false` not, because it already is 
a keyword in C++98.

I am against allowing `#define DEFAULTED` because both are perfectly valid and 
we ship a check for modernizing from one to the other version. This is 
different to `OVERRIDE`, because it adds additional value for the programmer 
who might be stuck at C++98.

>> 4. Debugging/Logging stuff that contains the Line and file. These can not be 
>> modeled with current C++(?), but `LineInfo` or similar exists. I don't know 
>> what its status is. I think it is ok to require manual silencing for such 
>> macros.
> 
> In addition to `__LINE__` and `__FILE__`, I think we want to include 
> `__DATE__`, `__TIME__`, and `__func__`.

Agreed.

> I think this list is a good start, but is likely incomplete. We'll probably 
> discover other things to add to the list when testing the check against other 
> real world code bases to see what patterns emerge from them.

Testing Frameworks are a common source for macro usage too. They should be 
coverable with the regex approach, but I don't know if there are other things 
hidden behind the interface that would be super annoying to silence.
Same for benchmark libraries.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129893.
JonasToth added a comment.

- address eugene comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 2 inline comments as done.
JonasToth added inline comments.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:8
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 
`_,

Eugene.Zelenko wrote:
> Will be good idea to separate statements with empty line.
As a list or how do you mean? 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129890.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.

- doc fix


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Core Guidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129889.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: macro used to declare a constant; consider using a 'constexpr' constant
+
+#define PROBLEMATIC_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: function like macro used; consider a (constexpr) template function
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: variadic macro used; consider using a variadic template
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x, y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.h:15
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {

Please include .



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:8
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 
`_,

Will be good idea to separate statements with empty line.



Comment at: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp:17
+
+
+#define DEBUG_CONSTANT 0

Unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129886.
JonasToth added a comment.

- [Feature] implement regex for allowed macros
- [Test] add proper tests for regex silencing


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage-custom.cpp
@@ -0,0 +1,26 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: cppcoreguidelines-macro-usage.AllowedRegexp, value: "DEBUG_*|TEST_*"}]}' --
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+
+#define DEBUG_CONSTANT 0
+#define DEBUG_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+#define DEBUG_VARIADIC(...) (__VA_ARGS__)
+
+#define TEST_CONSTANT 0
+#define TEST_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+#define TEST_VARIADIC(...) (__VA_ARGS__)
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,44 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41648#969418, @JonasToth wrote:

> > Yes, and I'm saying that the guidelines aren't useful for real code bases 
> > because they restrict more than is reasonable. So while I think the check 
> > is largely implementing what the guidelines recommend, I think that some of 
> > these scenarios should be brought back to the guideline authors to weigh in 
> > on before we expose the check to users.
>
> I see. Are u willing to identify some of the exceptions with me or don't you 
> have enough time? I would like to propose the first list here. My opinion is, 
> that we should leave some useful macros (e.g. `Ensure` from guidelines) left 
> for manual inspection and to disable the check for these specific macros.


I can try to help as I have time.

>> I don't think a whitelist is going to cut it here...
> 
> The whitelist could be a regex or something similar. Configuring the check to 
> allow everything with a prefix would allow poor mans namespaces for macros. 
> For llvm it would be `LLVM_*`, blaze could be `BLAZE_*`. These are the bigger 
> codebases i know where macro usage was reasonable. :)

Using regex could be a reasonable way out, but isn't going to solve the problem 
in general because not all of the macro names are ones the user has control 
over. Having to whitelist dozens of macros by name means this check is simply 
going to be disabled by the user as being low-value.

>> ... -- users are not going to try to figure out every conditional 
>> compilation or attribute macro definition used in their large code base;
> 
> Conditional compilation can be done with empty macros.
> 
>   #define ALLOW_EXCEPTION
>   
>   #ifdef ALLOW_EXCEPTION
>   // exception code
>   #else
>   // no exception code
>   #endif
> 
> 
> That usecase is in my eyes pretty much like an include guard an therfore 
> acceptable, and i think not diagnosed with the check currently.

Except that's not the only form of conditional compilation I'm worried about. 
See Compiler.h where we do things like `#define __has_attribute(x) 0`, which is 
a perfectly reasonable macro to define (and is an example of a macro name 
outside of the user's control, so they cannot simply prefix it for a whitelist).

>> For example, look at Compiler.h in LLVM and try to see how you would rewrite 
>> that code to perform the same purposes without triggering diagnostics from 
>> this check. That sort of file is common to almost every production code base 
>> I've ever come across.
>>  One way to make this less chatty would be to check whether the macro 
>> replacement list is defining a source construct that cannot be replaced by 
>> constexpr variables or inline functions (such as 
>> LLVM_ATTRIBUTE_ALWAYS_INLINE). If we had whole-program analysis, I think we 
>> could do something further like scan the uses of the macros to determine 
>> whether they're used for conditional compilation (such as 
>> LLVM_ENABLE_EXCEPTIONS). However, I have no idea how we would handle cases 
>> like LLVM_LIKELY and LLVM_UNLIKELY, but expecting users to NOLINT 500+ lines 
>> of common macro code doesn't seem like a good first approach. I'd be curious 
>> to know what the C++ Core Guidelines folks think about those use cases and 
>> how they would recommend rewriting the code to adhere to their guidelines. 
>> Do they really expect users to use *no* macros in C++ code outside of header 
>> include guards and noop definitions even when asked about reasonable use 
>> cases like cross-compiler attributes or builtins? I mean, many of these 
>> things cannot even use the attribute the C++ Core Guidelines require for 
>> silencing such diagnostics -- how do they want to use gsl::suppress to 
>> silence a diagnostic on a macro definition?
> 
> Common constructs that I would identify as valid:
> 
> 1. Compiler specific Attributes. `#define ALWAYS_INLINE 
> __attribute__((always_inline))` Attributes could be identified when 
> inspecting the tokens the macro expands to. I have no clue how to implement, 
> but i think that should even possible with string manipulations.

Agreed.

> 2. Compiler intrinsics These are similar to attributes and could maybe even 
> treated the same.

I'm less certain about this one -- with attributes, there's syntax that we can 
inspect (is there an `__attribute__` or `__declspec` keyword?), but with 
intrinsics there's no commonality. Or are you thinking of tablegenning the list 
of intrinsics?

> 3. Compatibility with older C++ Standards Stuff like `#define OVERRIDE 
> override` if the code is compiled with C++11 or newer might be acceptable for 
> stepwise modernization. This can can be considered as program text 
> manipulation that was explicitly forbidden but does not obscure code so an 
> exception could be made. Such macros can be detected with string comparisons.

How would you generalize this? I've seen people use macros for arbitrary 
keywords (`#define CONST const` and `#define false 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129691.
JonasToth added a comment.

- minor issues fixed


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,12 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+};
+
+} // namespace 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 129690.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,12 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+The relevant sections in the C++ Coreguidelines are 
+`Enum.1 `_,
+`ES.30 `_,
+`ES.31 `_ and
+`ES.33 `_.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,13 +57,20 @@
 Improvements to clang-tidy
 --
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `fuchsia-statically-constructed-objects
   `_ check
 
   Warns if global, non-trivial objects with static storage are constructed, unless the 
   object is statically initialized with a ``constexpr`` constructor or has no 
   explicit constructor.
 
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Yes, and I'm saying that the guidelines aren't useful for real code bases 
> because they restrict more than is reasonable. So while I think the check is 
> largely implementing what the guidelines recommend, I think that some of 
> these scenarios should be brought back to the guideline authors to weigh in 
> on before we expose the check to users.

I see. Are u willing to identify some of the exceptions with me or don't you 
have enough time? I would like to propose the first list here. My opinion is, 
that we should leave some useful macros (e.g. `Ensure` from guidelines) left 
for manual inspection and to disable the check for these specific macros.

> I don't think a whitelist is going to cut it here...

The whitelist could be a regex or something similar. Configuring the check to 
allow everything with a prefix would allow poor mans namespaces for macros. For 
llvm it would be `LLVM_*`, blaze could be `BLAZE_*`. These are the bigger 
codebases i know where macro usage was reasonable. :)

> ... -- users are not going to try to figure out every conditional compilation 
> or attribute macro definition used in their large code base;

Conditional compilation can be done with empty macros.

  #define ALLOW_EXCEPTION
  
  #ifdef ALLOW_EXCEPTION
  // exception code
  #else
  // no exception code
  #endif

That usecase is in my eyes pretty much like an include guard an therfore 
acceptable, and i think not diagnosed with the check currently.

> For example, look at Compiler.h in LLVM and try to see how you would rewrite 
> that code to perform the same purposes without triggering diagnostics from 
> this check. That sort of file is common to almost every production code base 
> I've ever come across.
>  One way to make this less chatty would be to check whether the macro 
> replacement list is defining a source construct that cannot be replaced by 
> constexpr variables or inline functions (such as 
> LLVM_ATTRIBUTE_ALWAYS_INLINE). If we had whole-program analysis, I think we 
> could do something further like scan the uses of the macros to determine 
> whether they're used for conditional compilation (such as 
> LLVM_ENABLE_EXCEPTIONS). However, I have no idea how we would handle cases 
> like LLVM_LIKELY and LLVM_UNLIKELY, but expecting users to NOLINT 500+ lines 
> of common macro code doesn't seem like a good first approach. I'd be curious 
> to know what the C++ Core Guidelines folks think about those use cases and 
> how they would recommend rewriting the code to adhere to their guidelines. Do 
> they really expect users to use *no* macros in C++ code outside of header 
> include guards and noop definitions even when asked about reasonable use 
> cases like cross-compiler attributes or builtins? I mean, many of these 
> things cannot even use the attribute the C++ Core Guidelines require for 
> silencing such diagnostics -- how do they want to use gsl::suppress to 
> silence a diagnostic on a macro definition?

Common constructs that I would identify as valid:

1. Compiler specific Attributes. `#define ALWAYS_INLINE 
__attribute__((always_inline))`

Attributes could be identified when inspecting the tokens the macro expands to. 
I have no clue how to implement, but i think that should even possible with 
string manipulations.

2. Compiler intrinsics

These are similar to attributes and could maybe even treated the same.

3. Compatibility with older C++ Standards

Stuff like `#define OVERRIDE override` if the code is compiled with C++11 or 
newer might be acceptable for stepwise modernization.
This can can be considered as program text manipulation that was explicitly 
forbidden but does not obscure code so an exception could be made.
Such macros can be detected with string comparisons.

4. Debugging/Logging stuff that contains the Line and file.

These can not be modeled with current C++(?), but `LineInfo` or similar exists. 
I don't know what its status is. I think it is ok to require manual silencing 
for such macros.

@aaron.ballman Do you agree with the list? I would ask the guideline authors 
about the issue and propose exceptions to the strict rules.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128869.
JonasToth added a comment.

- rebase after release for 6.0


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,8 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,7 +57,11 @@
 Improvements to clang-tidy
 --
 
-- ...
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
 
 Improvements to include-fixer
 -
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -0,0 +1,62 @@
+//===--- MacroUsageCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:13
+#include "clang/Lex/PPCallbacks.h"
+
+namespace clang {

Please include string and STLExtras.h.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:8
+constructs exist for the task.
+

Will be good idea to add link to relevant section in documentation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41648#965437, @JonasToth wrote:

> In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote:
>
> > I think this check is going to be extraordinarily chatty. For instance, 
> > macros are often used to hide compiler details in signatures, such as use 
> > of attributes. This construct cannot be replaced with anything else because 
> > the macro isn't defining an object or value. Another example where this 
> > will be bad is for conditional processing where the macro is later used in 
> > a `#if`, `#elif`, `#ifdef`, or `#ifndef` preprocessor conditional, as this 
> > also cannot be replaced with a `constexpr` variable. Without handling 
> > things like this, I don't see how this rule can provide utility to real 
> > world code. Do you have ideas for how to handle these kind of situations?
>
>
> The check will only warn in the definition of the macro not the expansion.


I understand. It's the definitions I am concerned about.

> The guidelines are really strict with macros and explicitly state that 
> program maniupulation is a bad thing.

Yes, and I'm saying that the guidelines aren't useful for real code bases 
because they restrict more than is reasonable. So while I think the check is 
largely implementing what the guidelines recommend, I think that some of these 
scenarios should be brought back to the guideline authors to weigh in on before 
we expose the check to users.

> Having a macro without value, like header guards or compile time flag style 
> things are not reported with this check.
> 
> Other (valid) use cases require a NOLINT with this check. But i do not know 
> how to figure out automatically what a macro is meant to do. I can introduce 
> a whitelist for allowed macros.
>  Furthermore i could make each use case a configurable option. E.g. 
> forbidding constant definitions i still a good thing (imho).

I don't think a whitelist is going to cut it here -- users are not going to try 
to figure out every conditional compilation or attribute macro definition used 
in their large code base; they'll simply disable the check as not being overly 
useful. For example, look at Compiler.h in LLVM and try to see how you would 
rewrite that code to perform the same purposes without triggering diagnostics 
from this check. That sort of file is common to almost every production code 
base I've ever come across.

One way to make this less chatty would be to check whether the macro 
replacement list is defining a source construct that cannot be replaced by 
constexpr variables or inline functions (such as LLVM_ATTRIBUTE_ALWAYS_INLINE). 
If we had whole-program analysis, I think we could do something further like 
scan the uses of the macros to determine whether they're used for conditional 
compilation (such as LLVM_ENABLE_EXCEPTIONS). However, I have no idea how we 
would handle cases like LLVM_LIKELY and LLVM_UNLIKELY, but expecting users to 
NOLINT 500+ lines of common macro code doesn't seem like a good first approach. 
I'd be curious to know what the C++ Core Guidelines folks think about those use 
cases and how they would recommend rewriting the code to adhere to their 
guidelines. Do they really expect users to use *no* macros in C++ code outside 
of header include guards and noop definitions even when asked about reasonable 
use cases like cross-compiler attributes or builtins? I mean, many of these 
things cannot even use the attribute the C++ Core Guidelines require for 
silencing such diagnostics -- how do they want to use gsl::suppress to silence 
a diagnostic on a macro definition?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote:

> I think this check is going to be extraordinarily chatty. For instance, 
> macros are often used to hide compiler details in signatures, such as use of 
> attributes. This construct cannot be replaced with anything else because the 
> macro isn't defining an object or value. Another example where this will be 
> bad is for conditional processing where the macro is later used in a `#if`, 
> `#elif`, `#ifdef`, or `#ifndef` preprocessor conditional, as this also cannot 
> be replaced with a `constexpr` variable. Without handling things like this, I 
> don't see how this rule can provide utility to real world code. Do you have 
> ideas for how to handle these kind of situations?


The check will only warn in the definition of the macro not the expansion. The 
guidelines are really strict with macros and explicitly state that program 
maniupulation is a bad thing.
Having a macro without value, like header guards or compile time flag style 
things are not reported with this check.

Other (valid) use cases require a NOLINT with this check. But i do not know how 
to figure out automatically what a macro is meant to do. I can introduce a 
whitelist for allowed macros.
Furthermore i could make each use case a configurable option. E.g. forbidding 
constant definitions i still a good thing (imho).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I think this check is going to be extraordinarily chatty. For instance, macros 
are often used to hide compiler details in signatures, such as use of 
attributes. This construct cannot be replaced with anything else because the 
macro isn't defining an object or value. Another example where this will be bad 
is for conditional processing where the macro is later used in a `#if`, 
`#elif`, `#ifdef`, or `#ifndef` preprocessor conditional, as this also cannot 
be replaced with a `constexpr` variable. Without handling things like this, I 
don't see how this rule can provide utility to real world code. Do you have 
ideas for how to handle these kind of situations?




Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:21
+public:
+  MacroUsageCallbacks(MacroUsageCheck *Check) : Check{Check} {}
+  void MacroDefined(const Token ,

I'd use `Check(Check)` instead of curly braces.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:28
+
+Check->warnMacro(MD);
+  }

I don't think this warning would be appropriate in all language modes. For 
instance, macros in C code, or in C++98 mode, etc.



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:42
+void MacroUsageCheck::warnMacro(const MacroDirective *MD) {
+  std::string DiagnosticMessage = "don't use macros to declare constants";
+

These diagnostics should be reworded to not use contractions and instead 
explain what the issue is. e.g., macro used to define a constant; consider 
using 'constexpr' instead (or something along those lines).



Comment at: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:44
+
+  auto *Info = MD->getMacroInfo();
+  if (Info->isFunctionLike())

Don't use `auto`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648



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

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128373.
JonasToth added a comment.

- remove unneeded includes


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,8 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -125,6 +125,12 @@
   a memory allocation function (``malloc()``, ``calloc()``, ``realloc()``,
   ``alloca()``) or the ``new[]`` operator in `C++`.
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `cppcoreguidelines-owning-memory `_ check 
 
   This check implements the type-based semantic of ``gsl::owner``, but without
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -0,0 +1,55 @@
+//===--- MacroUsageCheck.cpp - clang-tidy--===//
+//
+// The 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 128372.
JonasToth added a comment.

- merge with local repos


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,8 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -125,6 +125,12 @@
   a memory allocation function (``malloc()``, ``calloc()``, ``realloc()``,
   ``alloca()``) or the ``new[]`` operator in `C++`.
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `cppcoreguidelines-owning-memory `_ check 
 
   This check implements the type-based semantic of ``gsl::owner``, but without
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
@@ -0,0 +1,62 @@
+//===--- MacroUsageCheck.cpp - clang-tidy--===//
+//
+// The LLVM 

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, hokein, alexfh.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai, 
klimek.

In short macros are discouraged by multiple rules (and sometimes reference 
randomly).
This check allows only headerguards and empty macros for annotation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D41648

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp
  clang-tidy/cppcoreguidelines/MacroUsageCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-macro-usage.cpp

Index: test/clang-tidy/cppcoreguidelines-macro-usage.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-macro-usage.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-macro-usage %t
+
+#ifndef INCLUDE_GUARD
+#define INCLUDE_GUARD
+
+#define PROBLEMATIC_CONSTANT 0
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to declare constants
+
+#define PROBLEMATIC_FUNCTION(x,y) ((a) > (b) ? (a) : (b))
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate functions
+
+#define PROBLEMATIC_VARIADIC(...) (__VA_ARGS__)
+// CHECK-MESSAGES: [[@LINE-1]]:9: warning: don't use macros to simulate variadic templates
+
+#endif
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -54,6 +54,7 @@
cert-oop11-cpp (redirects to performance-move-constructor-init) 
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-macro-usage
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst
@@ -0,0 +1,8 @@
+.. title:: clang-tidy - cppcoreguidelines-macro-usage
+
+cppcoreguidelines-macro-usage
+=
+
+Find macro usage that is considered problematic because better language
+constructs exist for the task.
+
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -125,6 +125,12 @@
   a memory allocation function (``malloc()``, ``calloc()``, ``realloc()``,
   ``alloca()``) or the ``new[]`` operator in `C++`.
 
+- New `cppcoreguidelines-macro-usage
+  `_ check
+
+  Find macro usage that is considered problematic because better language
+  constructs exist for the task.
+
 - New `cppcoreguidelines-owning-memory `_ check 
 
   This check implements the type-based semantic of ``gsl::owner``, but without
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/MacroUsageCheck.h
@@ -0,0 +1,38 @@
+//===--- MacroUsageCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
+
+#include "../ClangTidy.h"
+#include "clang/Lex/Preprocessor.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+enum class MacroUsageMode { Constant, Function, Variadic };
+/// Find macro usage that is considered problematic because better language
+/// constructs exist for the task.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-macro-usage.html
+class MacroUsageCheck : public ClangTidyCheck {
+public:
+  MacroUsageCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerPPCallbacks(CompilerInstance ) override;
+  void warnMacro(const MacroDirective *MD);
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MACROUSAGECHECK_H
Index: clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp