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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits