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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
