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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
