[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 5 inline comments as done.
LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:382
+
+  if (std::strncmp("pragma", Text, 6) != 0)
+    return;
----------------
carlosgalvezp wrote:
> Maybe wrap the raw strings inside a StringRef for a more robust and readable comparison? Not a big fan of the magic 6 there :) 
OK, I've added some intention revealing functions that I think clean this up.  I'm testing now and will upload a diff when the tests pass.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:9-13
+
+This check can be used to enforce the C++ core guideline `Enum.1:
+Prefer enumerations over macros
+<https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum1-prefer-enumerations-over-macros>`_,
+within the constraints outlined below.
----------------
carlosgalvezp wrote:
> Oh, it's right here :) I suppose as a user I would expect to find this info in the cppcoreguidelines doc, not here. But again I don't know what the de-facto pattern is with aliases so I'll leave that to someone that knows better.
[[ https://clang.llvm.org/extra/clang-tidy/checks/readability-magic-numbers.html | readability-magic-numbers ]] is similar; the alias simply links to the readability check and the readability check cites the C++ Core Guideline with a link.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:19
+- Macros must be defined on sequential source file lines, or with
+  only comment lines in between macro definitions.
+- Macros must all be defined in the same source file.
----------------
carlosgalvezp wrote:
> Hmm, what about this situation?
> 
> 
> ```
> // This is some macro
> #define FOO 123
> // This is some other unrelated macro
> #define BAR 321
> ```
> 
> Would the check group these 2 together? Personally I'd put an empty line between the two to show they are unrelated, but I know many people prefer to save vertical space and keep everything tight without empty lines.
They're "grouped" into the same anonymous enum.  The basic heuristic is that blank lines separate groups of related identifiers, not comment lines.  This is the most common pattern in header files with macros that I've observed for decades.


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

https://reviews.llvm.org/D117522