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

dyung added a comment. I've reverted this change in order to get the build bots green again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 ___ cfe-commits mai

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

RKSimon added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:216 + MacroToEnumCheck *Check; + const LangOptions &LangOptions; + const SourceManager &SM; @LegalizeAdulthood I'm seeing errors in https://lab.llvm.org/bu

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG39b80c8380c8: [clang-tidy] Add modernize-macro-to-enum check (authored by LegalizeAdulthood). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ http

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Thanks for the discussion on this new check, it LGTM! Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; -

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

LegalizeAdulthood updated this revision to Diff 418112. LegalizeAdulthood added a comment. - clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt clang-tools-extra/clang-

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

LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; aaron.ballman wrote: > LegalizeAdulthood wrote: > > aaron.ballman

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; LegalizeAdulthood wrote: > aaron.ballman wrote: > > LegalizeAdulthood wrote: > > > aaron.ballman wrote: > > > > LegalizeA

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

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; aaron.ballman wrote: > LegalizeAdulthood wrote: > > aaron.ballman wrote: > > > LegalizeAdulthood wrote: > > > > aaron

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; LegalizeAdulthood wrote: > aaron.ballman wrote: > > LegalizeAdulthood wrote: > > > aaron.ballman wrote: > > > > I'm a bit

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

LegalizeAdulthood updated this revision to Diff 416627. LegalizeAdulthood added a comment. - Undo change introduced by clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

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

LegalizeAdulthood marked an inline comment as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:318 + + // It can be +Lit, -Lit or just Lit. + Token Tok = Info->tokens().front(); LegalizeAdult

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

LegalizeAdulthood updated this revision to Diff 416625. LegalizeAdulthood added a comment. - Recognize bitwise negated integers CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt cl

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

LegalizeAdulthood updated this revision to Diff 416624. LegalizeAdulthood added a comment. - Tests pass again CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt clang-tools-extra/cl

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

LegalizeAdulthood added a comment. In D117522#3392122 , @aaron.ballman wrote: > In D117522#3390136 , > @LegalizeAdulthood wrote: > >> I think I've got all the changes incorporated, but I'm getting a test >> fai

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

LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; aaron.ballman wrote: > LegalizeAdulthood wrote: > > aaron.ballman

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

LegalizeAdulthood updated this revision to Diff 416550. LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added a comment. - Update from review comments - `check-clang-tools` is reporting a test failure that still needs to be diagnosed (I think it is a mismatch between a CHECK

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

LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:315 + if (Info->isFunctionLike() || Info->isBuiltinMacro() || + Info->tokens().empty() || Info->tokens().size

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

LegalizeAdulthood added a comment. I've worked through this issue before, I just need to spend some time on it. The basic problem is that the test fails and dumps a bunch of output to "help" you understand the failure, but the way it is formatted and mangled doesn't end up being useful. CHANGE

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

aaron.ballman added a comment. In D117522#3390136 , @LegalizeAdulthood wrote: > I think I've got all the changes incorporated, but I'm getting a test failure > so I haven't uploaded a new diff. > > Honestly, I don't understand the test failure output.

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

LegalizeAdulthood added a comment. I think I've got all the changes incorporated, but I'm getting a test failure so I haven't uploaded a new diff. Honestly, I don't understand the test failure output. On Windows, the test failure output is all mangled and difficult to interpret correctly as to

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

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; LegalizeAdulthood wrote: > aaron.ballman wrote: > > I'm a bit confused by this one as this is not a valid line ending (it

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

LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68 + +// Undefining a macro invalidates adjacent macros +// from being considered as an enum. +#defi

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

LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:47 +CRLF, +CRLFCR, + }; aaron.ballman wrote: > I'm a bit confused by this one as this is

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

aaron.ballman added a comment. In D117522#3364387 , @LegalizeAdulthood wrote: > Ping. Another week waiting for reviews... Thanks for the ping! FWIW, it's also not uncommon for there to be week delays (reviewers go on vacation, have other obligations,

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

LegalizeAdulthood added a comment. Herald added a project: All. Ping. Another week waiting for reviews... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 ___ cfe-commits mailing list cfe-commits@lis

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

LegalizeAdulthood updated this revision to Diff 411555. LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added a comment. - Add intention revealing functions for details of pragma once parsing CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews

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

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

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

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: t

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

LegalizeAdulthood added a comment. Ping. Waiting for review feedback for about a week now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

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

LegalizeAdulthood updated this revision to Diff 408235. LegalizeAdulthood added a comment. - Move small methods to class decl CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt clan

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

LegalizeAdulthood updated this revision to Diff 408233. LegalizeAdulthood added a comment. - clang-format CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt clang-tools-extra/clang-

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

LegalizeAdulthood updated this revision to Diff 408231. LegalizeAdulthood added a comment. Herald added subscribers: kbarton, nemanjai. - Rejects macros that are used in preprocessor conditionals - cppcoreguidelines-macro-to-enum is an alias for modernize-macro-to-enum - Document alias CHANGES S

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

LegalizeAdulthood added a comment. Mmm... just realized that this check should invalidate any macro that is used in a conditional compilation expression, because enums can't be used there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 __

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

LegalizeAdulthood added a comment. In D117522#3299423 , @carlosgalvezp wrote: > I'm mostly interested on the warning message, [...] I'm open to changes in the wording of the warning message. > I get an error when downloading, do you happen to know what

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

carlosgalvezp added a comment. Ok, thanks for the explanation! I'm mostly interested on the warning message, we've had situations before where the warning describes the problem **and** the solution, which can easily lead to confusion. From the tests I can see the message is quite generic "use a

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

LegalizeAdulthood added a comment. In D117522#3290604 , @carlosgalvezp wrote: > This will conflict with `Enum.3` from `cppcoreguidelines` I went back and looked at Enum.3 Prefer class enums over “plain” enums

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

LegalizeAdulthood added a comment. Oh, I think I misinterpreted what you were saying in the last bit: In D117522#3290604 , @carlosgalvezp wrote: > Another point is that macros typically have a different naming > convention than enums or constants, so us

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

LegalizeAdulthood added a comment. I think it helps to understand this check better if we consider what we would do manually if we were modernizing code with a lot of macro-style enums in it. I've done this myself; in fact all the checks I've written for clang-tidy have been motivated by moderniz

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

carlosgalvezp added a comment. Since this is a `modernize` check, shouldn't it be proposing to use `enum class` instead of `enum`? This will conflict with `Enum.3` from `cppcoreguidelines`, and I don't think it's unreasonable that people enable both `modernize*` and `cppcoreguidelines*`. Not su

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

LegalizeAdulthood added a comment. Ping. This check has gone for a week without review comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

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

LegalizeAdulthood added a comment. This situation isn't properly diagnosed (false positive): #ifdef USE_FOO #if USE_FOO // code #endif #endif CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 __

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

LegalizeAdulthood updated this revision to Diff 402725. LegalizeAdulthood added a comment. Remove -std for test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modernize/CMakeLists.txt clang-tools-extra/

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

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:1 +// RUN: %check_clang_tidy -std c++14-or-later %s modernize-macro-to-enum %t -- -- -I%S/Inputs/modernize-macro-to-enum + Ah, this leake

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

LegalizeAdulthood updated this revision to Diff 402716. LegalizeAdulthood added a comment. Update from additional testing: - reject floating-point literals (they can't be enumerations) - add more test cases CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.

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

LegalizeAdulthood updated this revision to Diff 402416. LegalizeAdulthood added a comment. Switch back to SmallVector after debugging with std::vector Drop unnecessary includes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-too

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

LegalizeAdulthood updated this revision to Diff 402415. LegalizeAdulthood added a comment. Testing on iterated dynamics revealed a number of shortcomings; update the code to address those: - Improve handling of include guards - Handle conditional compilation blocks better - Reject macros that exp

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

LegalizeAdulthood updated this revision to Diff 401414. LegalizeAdulthood added a comment. - Tweak documentation, make sure sphinx runs without errors CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/modern

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

LegalizeAdulthood updated this revision to Diff 401299. LegalizeAdulthood added a comment. - clang-format - prefer `llvm::any_of` over `std::any_of` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://reviews.llvm.org/D117522 Files: clang-tools-extra/clang-tidy/moderniz

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

LegalizeAdulthood updated this revision to Diff 401238. LegalizeAdulthood added a comment. - Handle macros inside header files with include guards - Track state per-file and push/pop that state as files are included CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://revie

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

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:182 + MD->getMacroInfo()->isUsedForHeaderGuard() || + MD->getMacroInfo()->isBuiltinMacro() || ConditionScope > 0) +return; LegalizeAdult

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

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:182 + MD->getMacroInfo()->isUsedForHeaderGuard() || + MD->getMacroInfo()->isBuiltinMacro() || ConditionScope > 0) +return; LegalizeAdult

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

LegalizeAdulthood updated this revision to Diff 400700. LegalizeAdulthood added a comment. Addresses most of the review comments, still need to: - Verify that include guards don't interfere with analysis of headers CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117522/new/ https://revie

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

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:192-194 + if (LastFile != CurrentFile) { +LastFile = CurrentFile; +newEnum(); LegalizeAdulthood wrote: > njames93 wrote: > > This seems a str

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

LegalizeAdulthood marked 13 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:134 + +bool MacroToEnumCallbacks::isConsecutiveMacro(const MacroDirective *MD) const { + if (LastMacroLocation.is

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

njames93 added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/modernize-macro-to-enum.rst:45 + enum { + RED = 0xFF, + GREEN = 0x00FF00, Eugene.Zelenko wrote: > It'll be reasonable to support indentation. Option could be used to spec

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

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.h:19 + +/// FIXME: Write a short description. +/// njames93 wrote: > FIXME Please do this :-) Comment at: clang-tools-extra/clang-tidy/mod

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

njames93 added a comment. How does this handle cases where a macro is Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:106 + +bool hasBlankLines(const std::string &Text) { + enum class WhiteSpaceState { Comment at: cl

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

LegalizeAdulthood added a comment. RFC: Should we create a cppcoreguidelines alias since this implements "Enum.1: Prefer enumerations over macros "? This check tries to be very conservative so as to not generate false posi

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

LegalizeAdulthood created this revision. LegalizeAdulthood added reviewers: alexfh, njames93, aaron.ballman, JonasToth. LegalizeAdulthood added a project: clang-tools-extra. Herald added subscribers: carlosgalvezp, xazax.hun, mgorny. LegalizeAdulthood requested review of this revision. This check