[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 &MacroNameTok,
----------------
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