[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23 -namespace { - -bool isCapsOnly(StringRef Name) { - return std::all_of(Name.begin(), Name.end(), [](const char C) { -if (std::isupper(C) ||

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

This revision was automatically updated to reflect the committed changes. LegalizeAdulthood marked an inline comment as done. Closed by commit rGd83ecd77cc0f: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants (authored by LegalizeAdulthood). Changed prior to commit:

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood marked 3 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11 +`ES.31

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23 -namespace { - -bool isCapsOnly(StringRef Name) { - return std::all_of(Name.begin(), Name.end(),

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11 +`ES.31 `_, and +`ES.32

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp accepted this revision. carlosgalvezp added a comment. This revision is now accepted and ready to land. Looks reasonable to me! I'm fairly new here so I might not be the most "authoritative reviewer", let me know if you'd like someone else to give the final approval

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood updated this revision to Diff 401242. LegalizeAdulthood marked an inline comment as done. LegalizeAdulthood added a comment. - Update from comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386 Files:

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-macro-usage.rst:11 +`ES.31

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:23 -namespace { - -bool isCapsOnly(StringRef Name) { - return std::all_of(Name.begin(), Name.end(), [](const char C) { -if (std::isupper(C) ||

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood updated this revision to Diff 401087. LegalizeAdulthood added a comment. - For function `isCapsOnly`: - Declare as static per LLVM style guide - Simplify boolean expression - Use `llvm::all_of` on container - Inline Function `isLiteralTokenSequence` as it is now simpler

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. In D116386#3252351 , @carlosgalvezp wrote: > Would it be worth mentioning the change in the release notes? [...] Updating the documentation based on the discussion thread and the release notes are both good ideas,

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:85 + +bool isLiteralTokenSequence(const MacroInfo *Info) { + return std::all_of(Info->tokens_begin(), Info->tokens_end(), LLVM coding standards

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

carlosgalvezp added a comment. Code looks great! Would it be worth mentioning the change in the release notes? I also wonder if the check documentation needs some updates. From what i read in the discussion this rule has poor enforcement in the guidelines so perhaps it's good to clarify what

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. Ping again. This change isn't that long or complicated and fixes a bug that results in false positives from this check. Please give it a review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. Ping. This review has been waiting for a week without any comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386 ___ cfe-commits mailing list

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood updated this revision to Diff 399186. LegalizeAdulthood added a comment. - clang-format - use Token::isLiteral instead of homebrew token predicate - rename predicate on token list to reflect isLiteral CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a reviewer: JonasToth. LegalizeAdulthood added a comment. I'm adding the original author of this check as a reviewer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386 ___

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. In D116386#3227236 , @aaron.ballman wrote: > We're in strong agreement that guideline checkers with untenable false > positive rates are a > very bad thing. I think this particular check is of insufficient quality to

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

aaron.ballman added a comment. In D116386#3226174 , @LegalizeAdulthood wrote: > Aaron, I think your comments are useful and I would be inclined to agree with > you if I > was the original author of this check. Sorry, I hope I didn't give the

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. Also, re-reading the C++ guidelines, I agree that this check is handling the two cases ES.31 (Don't use macros for constants and functions) ES.33 (Don't use macros that aren't all caps) I guess previously it **technically** handled ES.30 (don't use macros for

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. Aaron, I think your comments are useful and I would be inclined to agree with you if I was the original author of this check. I treat the guidelines as just that: guidelines, not rules. In the context of clang-tidy I think you're correct that some

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

aaron.ballman edited reviewers, added: hokein, whisperity, njames93; removed: aaron.ballman. aaron.ballman added a comment. Herald added a subscriber: rnkovacs. We run into this problem quite frequently -- the C++ Core Guidelines put very little effort into thinking about enforcement, and so

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added a comment. Can someone look at this review please, it's not that long and/or complicated. Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/ https://reviews.llvm.org/D116386 ___ cfe-commits mailing

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood updated this revision to Diff 396683. LegalizeAdulthood marked an inline comment as not done. LegalizeAdulthood added a comment. Improve name of predicate function. Use standard algorithms instead of raw loops CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116386/new/

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/MacroUsageCheck.cpp:83-105 +bool isConstantToken(const MacroDirective *MD) { + for (const auto : MD->getMacroInfo()->tokens()) { +switch (Token.getKind()) { +case

[PATCH] D116386: [clang-tidy] Narrow cppguidelines-macro-usage to actual constants

LegalizeAdulthood created this revision. LegalizeAdulthood added a reviewer: alexfh. Herald added subscribers: carlosgalvezp, kbarton, xazax.hun, nemanjai. LegalizeAdulthood requested review of this revision. Herald added a project: clang-tools-extra. Previously, any macro that didn't look like a