aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:51-54
+ if (!getLangOpts().C99) {
+ return llvm::None;
+ }
+
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:69
+ // Only insert spaces if there aren't already spaces between operators
+ StringRef SpaceBefore = std::isspace(lookahead(SM, Loc, -1)) ? "" : " ";
+ StringRef SpaceAfter =
----------------
Hrmm, I feel like `std::isspace()` is not going to properly handle all the
Unicode whitespace, but I don't think we handle them all anyway as I notice the
lexer is using `isHorizontalWhitespace()`, `isVerticalWhitespace()`, and
`isWhitespace()` from `CharInfo.h`. Maybe we should use the same utility here
just to match the lexer? (I don't feel strongly, just tickled my spidey senses.)
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:78-79
+ const UnaryOperator &Op) {
+ // TODO: make check configurable so users can decide which operators are to
be
+ // symbols and which are to be words.
+
----------------
Are you planning to do this as part of this patch (if not, should this switch
to FIXME)?
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:85
+ assert(First != nullptr);
+ if (std::isalpha(*First) || Loc.isMacroID())
+ return;
----------------
`isLetter()`? (or potentially `isIdentifierHead()` if we need to care about
dollar-sign prefixed identifiers)
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:104
+ const BinaryOperator &Op) {
+ // TODO: make check configurable so users can decide which operators are to
be
+ // symbols and which are to be words.
----------------
Same here as above.
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:111
+ assert(First != nullptr);
+ if (std::isalpha(*First) || Loc.isMacroID())
+ return;
----------------
Same here as above.
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:117-120
+ case Opcode::BO_LAnd:
+ diag(Loc, "use 'and' for logical conjunctions")
+ << createReplacement(SM, Loc, "and", 2) << includeIso646(SM, Loc);
+ break;
----------------
I think these can be replaced with a helper function along these lines:
```
void emitDiag(const SourceManager &SM, SourceLocation Loc, StringRef Op,
StringRef OpDesc, int Lookahead) {
diag(Loc, ("use '" + Op + "' for " + OpDesc).str()) <<
createReplacement(SM, Loc, Op, Lookahead) << includeIso646(SM, Loc);
}
```
WDYT?
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:144-148
+ // TODO: make check configurable so users can decide which operators are to
be
+ // symbols and which are to be words.
+
+ // TODO: add check to make it possible to "intelligently" determine if symbol
+ // is always preferred (e.g. operator| being used for composition).
----------------
FIXMEs?
================
Comment at:
clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.cpp:158-164
+ constexpr Opcode And = Opcode::OO_AmpAmp;
+ constexpr Opcode Bitand = Opcode::OO_Amp;
+ constexpr Opcode Bitor = Opcode::OO_Pipe;
+ constexpr Opcode Compl = Opcode::OO_Tilde;
+ constexpr Opcode Not = Opcode::OO_Exclaim;
+ constexpr Opcode Or = Opcode::OO_PipePipe;
+ constexpr Opcode Xor = Opcode::OO_Caret;
----------------
Nit: given that these are only used once, I'd rather just spell out the `case`
labels longhand.
================
Comment at: clang-tools-extra/clang-tidy/readability/AlternativeTokensCheck.h:35
+ Preprocessor *ModuleExpanderPP) override {
+ // TODO: add support for checking preprocessor directives
+ Includer.registerPreprocessor(PP);
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.h:9
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRICTCONSTCORRECTNESS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRICTCONSTCORRECTNESS_H
----------------
Did you mean to include this file in this review?
================
Comment at:
clang-tools-extra/clang-tidy/readability/StrictConstCorrectness.kpp:1
+//===-- StringCompareCheck.cpp -
clang-tidy--------------------------------===//
+//
----------------
Same for this one?
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/readability-alternative-tokens.rst:4
+readability-alternative-tokens
+==================================
+
----------------
================
Comment at:
clang/test/Analysis/diagnostics/readability-strict-const-correctness.cpp:1
+// RUN: %check_clang_tidm1 %s readabilitm1-strict-const-correctness %t
+int f1()
----------------
No idea how this managed to trigger a CI failure of `line 1: fg: no job
control` :-D
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
