[PATCH] D148318: [clang-tidy] Add `performance-avoid-endl` check

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Overall looks good, didn't found any bugs, just some potential improvements.



================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:23
+void AvoidEndlCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      expr(unless(isExpansionInSystemHeader()),
----------------
Consider using TK_IgnoreUnlessSpelledInSource like in other checks, this should allow you to remove ignoringImplicit.


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:24
+  Finder->addMatcher(
+      expr(unless(isExpansionInSystemHeader()),
+           anyOf(cxxOperatorCallExpr(
----------------
cxxOperatorCallExpr inherits from callExpr.
Instead of using here expr, use callExpr, that should allow to filter more ast elements.


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:28
+                     hasRHS(ignoringImplicit(
+                         declRefExpr(to(namedDecl(hasName("endl"))))
+                             .bind("expr")))),
----------------
consider ::std::endl unless you plan to support any endl function


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:30
+                             .bind("expr")))),
+                 callExpr(unless(isExpansionInSystemHeader()),
+                          callee(functionDecl(hasName("endl"))),
----------------
`unless(isExpansionInSystemHeader())` is not needed here, you have it in line 24, thats sufficient.


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:31
+                 callExpr(unless(isExpansionInSystemHeader()),
+                          callee(functionDecl(hasName("endl"))),
+                          argumentCountIs(1))
----------------
same here consider ::std::endl


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:32
+                          callee(functionDecl(hasName("endl"))),
+                          argumentCountIs(1))
+                     .bind("expr"))),
----------------
put this `argumentCountIs` first, it will be cheaper to check than string compare.


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:48-51
+    const CharSourceRange TokenRange =
+        CharSourceRange::getTokenRange(Expression->getSourceRange());
+    const StringRef SourceText = Lexer::getSourceText(
+        TokenRange, *Result.SourceManager, Result.Context->getLangOpts());
----------------
entire reading of text is redundant, in log you can put simply:
`"do not use 'std::endl' with streams; use '\\n' instead"`


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:61
+    const auto *CallExpression = llvm::cast<CallExpr>(Expression);
+    assert(CallExpression->getNumArgs() == 1);
+
----------------
this shouldn't be needed, you already checked that you have 1 argument


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:63-66
+    const StringRef SourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(
+            CallExpression->getCallee()->getSourceRange()),
+        *Result.SourceManager, Result.Context->getLangOpts());
----------------
this again looks redundant..., just use std::endl in diag


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:68-70
+    auto Diag = diag(CallExpression->getBeginLoc(),
+                     "do not use '%0' with streams; use '\\n' instead")
+                << SourceText;
----------------
move this to line 80, you won't need to assign this to variable


================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.h:16
+
+/// ClangTidyCheck to flag all uses of std::endl on iostreams.
+///
----------------
sync this text with a description in release notes, and first sentence in documentation


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance/avoid-endl.rst:7
+Checks for uses of ``std::endl`` on streams and suggests using the newline
+character ``"\n"`` instead.
+
----------------
should be `'\n'`, same in documentation, and check comment


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance/avoid-endl.rst:40
+
+Additionally, it is important to note that the ``std::cerr`` and ``std::clog``
+streams always flush after a write operation, regardless of whether ``std::endl``
----------------
and std::cout, std::cerr, std::clog, std::wcout, std::wcerr and std::wclog.
simply use wording like "standard C++ streams" and then put some examples in ().


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance/avoid-endl.rst:41
+Additionally, it is important to note that the ``std::cerr`` and ``std::clog``
+streams always flush after a write operation, regardless of whether ``std::endl``
+or ``"\n"`` is used. Therefore, using ``"\n"`` with these streams will not
----------------
Unless std::ios_base::sync_with_stdio is set to false.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/performance/avoid-endl.rst:53
+    int main() {
+        std::cout << "Hello\n" << std::flush;
+    }
----------------
use 2 spaces instead of 4


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148318/new/

https://reviews.llvm.org/D148318