whisperity added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:61 +FixItHint AlternativeTokensCheck::createReplacement(SourceLocation Loc, + StringRef S, int N) const { + // Only insert spaces if there aren't already spaces between operators ---------------- What's `N`? It's not immediately apparent for me from the callsites of this function. ================ Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:70-71 +void AlternativeTokensCheck::checkSpelling(const UnaryOperator &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First) || Loc.isMacroID()) ---------------- Are we sure these will never return an invalid Loc, or dereference a null? Maybe it'd be worth to investigate, add an assertion (to help the people who might run static analysis on LLVM, if for nothing else), or an early return. ================ Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:90-91 +void AlternativeTokensCheck::checkSpelling(const BinaryOperator &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First) || Loc.isMacroID()) ---------------- Same comment as in the `UnaryOperator` case. ================ Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:124-125 +void AlternativeTokensCheck::checkSpelling(const CXXOperatorCallExpr &Op) { + SourceLocation Loc = Op.getOperatorLoc(); + char First = *(SM->getCharacterData(Loc)); + if (std::isalpha(First) || Loc.isMacroID()) ---------------- Ditto. (Usually there might be issues if the location is coming from generated code or templates or macros or something... I fear this could be mostly prevalent in the user-defined operator case, i.e. this method.) ================ Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h:20-21 +namespace readability { +/// Flags uses of symbol-based bitwise and logical operators. +class AlternativeTokensCheck : public ClangTidyCheck { +public: ---------------- Following from gone thread due to file rename. >>! In D107294#2923102, @cjdb wrote: > Not sure I'm following you here: are you suggesting I put the contents of my > `rst` file in a comment here? Not the entire //RST//, but the one-sentence or first-paragraph "pitch". For example, let's see `bugprone-branch-clone`'s class's doc-comment: ``` /// A check for detecting if/else if/else chains where two or more branches are /// Type I clones of each other (that is, they contain identical code), for /// detecting switch statements where two or more consecutive branches are /// Type I clones of each other, and for detecting conditional operators where /// the true and false expressions are Type I clones of each other. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-branch-clone.html class BranchCloneCheck : public ClangTidyCheck { ``` Or another one selected randomly, `performance-no-automatic-move`: ``` /// Finds local variables that cannot be automatically moved due to constness. /// /// For the user-facing documentation see: /// http://clang.llvm.org/extra/clang-tidy/checks/performance-no-automatic-move.html class NoAutomaticMoveCheck : public ClangTidyCheck { ``` So there is a one-paragraph summary of the check itself (it could be shorter than here...), and there is a text that's generated from a template (I think `add-new-check.py` sets the new check's files as such when you run it), which basically just links the upstream official website render of your check's documentation HTML. ================ Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:133-134 "readability-uppercase-literal-suffix"); + CheckFactories.registerCheck<UseAlternativeTokensCheck>( + "readability-use-alternative-tokens"); CheckFactories.registerCheck<UseAnyOfAllOfCheck>( ---------------- cjdb wrote: > aaron.ballman wrote: > > I think this might be a case where we want the check to either recommend > > using alternative tokens or recommend against using alternative tokens via > > a configuration option (so users can control readability in either > > direction). If you agree that's a reasonable design, then I'd recommend we > > name this `readability-alternative-tokens` to be a bit more generic. (Note, > > we can always do the "don't use alternative tokens" implementation in a > > follow-up patch if you don't want to do that work immediately.) > Hrmm.... I'll do the rename now, but this might be better off as a later > patch. I'd rather focus on getting what I have right (along with my teased > extensions) and then work on the opposite direction. That will (probably) be > an easy slip-in. (As someone who's had checkers on review for multiple years I've no hard feelings about the scheduling.) But please do add a few `FIXME:` or `TODO:` or `IDEA:` or some similar comments to the code somewhere about the suggested follow-ups. (Just so they don't go away when this review is closed.) ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst:6-7 + +Finds uses of symbol-based logical and bitwise operators and recommends using +alternative tokens instead. + ---------------- I think (for now, until the check is improved/extended) this first paragraph / oneliner added to the doccomment in the header shall suffice. It's only to ensure that the automatically generated documentation for the check's class has a comment from which documentation readers can look into what the check actually does (from a user's standpoint), by reaching this file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107294/new/ https://reviews.llvm.org/D107294 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits