[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

One big concern I have with this is the potential to introduce ODR violations into the user's code. We may want to limit this check to only trigger on macros that are defined in the primary source file of the TU rather than something in the include stack.

One other problem with this advice is that a constant variable isn't always the same thing as a macro replacement value. For instance, a const variable requires the ability to take the address of the variable, which means that space must be reserved for it at runtime. A frequent way around this is to use enumerator rather than constant variables for integral types. The enumerations are not prone to ODR violations and enumerators are not things with storage.



================
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:41
+
+/// numeric values may have + or - signs in front of them
+/// others like ~ are not so obvious and depend on usage
----------------
Make sure the comments have proper capitalization and punctuation, here and elsewhere.


================
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:43
+/// others like ~ are not so obvious and depend on usage
+static bool isReasonableNumberPrefix(const Token &T) {
+  return T.isOneOf(tok::plus, tok::minus);
----------------
I don't think most of these helper functions really clarify the code all that much (except for perhaps `isAnyCharLiteral()`, but even that's debatable). I would just fold these into the code.


================
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:83-84
+    }
+    // numbers may have a prefix, however chars with a prefix are rather
+    // strange... let's not touch them
+    else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) ||
----------------
I don't really agree with this comment. `L'1'` is a reasonable character constant and not at all strange. You should add tests for that case and perhaps clarify the comment.


================
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:86
+    else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) ||
+             (!hasPrefix && isAnyCharLiteral(Tok))) {
+      // prefix shall not be accepted anymore after any number
----------------
What about string literals? e.g., `#define NAME "foobar"`


================
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.h:19-21
+/// C++ const variables should be preferred over #define statements
+///
+/// const variables obey type checking and scopes
----------------
Missing punctuation at the end of the sentences in these comments.


================
Comment at: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst:14
+
+  `// calc.h
+  namespace RealCalculation {
----------------
Extraneous ` before the comment.


https://reviews.llvm.org/D29692