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