AMS21 added inline comments.

================
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());
----------------
PiotrZSL wrote:
> entire reading of text is redundant, in log you can put simply:
> `"do not use 'std::endl' with streams; use '\\n' instead"`
I did this before and it was suggested that we match the users spelling. See 
njames93 comment above.
But honestly I'm fine either way. Hard coding the `std::endl` definitely makes 
the code simpler.

> Wrap the `std::endl` in single quotes.
> 
> Also it may be wise to spell this how the user has it spelt
> 
> ```lang=c++
> std::cout << std::endl; // do not use 'std::endl'
> using namespace std;
> cout << endl; // do not use 'endl'




================
Comment at: clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp:61
+    const auto *CallExpression = llvm::cast<CallExpr>(Expression);
+    assert(CallExpression->getNumArgs() == 1);
+
----------------
PiotrZSL wrote:
> this shouldn't be needed, you already checked that you have 1 argument
That's true. I still like to write asserts like this, because the code in this 
code block requires the argument count to 1.  So if something would be changed 
in the future here, you would hopefully trip the assert and see that the code 
below needs to be adjusted.

And since this is only checked for debug builds, I don't see the harm here.


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

Reply via email to