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

Carlos Galvez via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Feb 20 03:03:49 PST 2022


carlosgalvezp added a comment.

I tried to download the diff and apply it from the root of llvm-project, but I must be doing something wrong...

  $ git apply D117522.diff
  D117522.diff:808: trailing whitespace.
                                  - attempt to free an illegal 
  D117522.diff:809: trailing whitespace.
                                    cmap entry 
  D117522.diff:810: trailing whitespace.
                                  - attempt to store into a read-only 
  D117522.diff:824: trailing whitespace.
  // CHECK-FIXES-NEXT:                                 - attempt to free an illegal 
  D117522.diff:825: trailing whitespace.
  // CHECK-FIXES-NEXT:                                   cmap entry 
  error: clang-tidy/modernize/CMakeLists.txt: No such file or directory
  error: clang-tidy/modernize/ModernizeTidyModule.cpp: No such file or directory
  error: docs/ReleaseNotes.rst: No such file or directory
  error: docs/clang-tidy/checks/list.rst: No such file or directory



================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:382
+
+  if (std::strncmp("pragma", Text, 6) != 0)
+    return;
----------------
Maybe wrap the raw strings inside a StringRef for a more robust and readable comparison? Not a big fan of the magic 6 there :) 


================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:389
+
+  if (std::strncmp("once", Text, 4) == 0)
+    CurrentFile->GuardScanner = IncludeGuard::IfGuard;
----------------
Same here


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-to-enum.rst:7-8
+===============================
+
+The cppcoreguidelines-macro-to-enum check is an alias, please see
+:doc:`modernize-macro-to-enum <modernize-macro-to-enum>` for more information.
----------------
Would it make sense to mention this check covers the rule [[ https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Renum-macro | Enum.1 ]]?

I see that we follow a standard pattern for aliases where we simply redirect to the main check, but maybe it's good to point this out anyway? Otherwise it might be unclear exactly which rule this check is referring to.

One drawback though is that aliases redirect to the main check after 5 seconds...


================
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.
----------------
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.


================
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.
----------------
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.


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

https://reviews.llvm.org/D117522



More information about the cfe-commits mailing list