Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] add default error message for performance-avoid-endl #107867

Conversation

HerrCai0907
Copy link
Contributor

@HerrCai0907 HerrCai0907 commented Sep 9, 2024

use ::std::endl as default message when matched expr does not have valid source text

Fixes: #107859

use ::std::endl as default message when matched expr does not have valid source text
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 9, 2024

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Congcong Cai (HerrCai0907)

Changes

use ::std::endl as default message when matched expr does not have valid source text


Full diff: https://github.com/llvm/llvm-project/pull/107867.diff

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp (+6-7)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/performance/avoid-endl.cpp (+11)
diff --git a/clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp b/clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp
index 8ecaa41754fb2b..4cfe1ca49f317c 100644
--- a/clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/AvoidEndlCheck.cpp
@@ -46,14 +46,13 @@ void AvoidEndlCheck::check(const MatchFinder::MatchResult &Result) {
     // Handle the more common streaming '... << std::endl' case
     const CharSourceRange TokenRange =
         CharSourceRange::getTokenRange(Expression->getSourceRange());
-    const StringRef SourceText = Lexer::getSourceText(
+    StringRef SourceText = Lexer::getSourceText(
         TokenRange, *Result.SourceManager, Result.Context->getLangOpts());
-
-    auto Diag = diag(Expression->getBeginLoc(),
-                     "do not use '%0' with streams; use '\\n' instead")
-                << SourceText;
-
-    Diag << FixItHint::CreateReplacement(TokenRange, "'\\n'");
+    if (SourceText.empty())
+      SourceText = "::std::endl";
+    diag(Expression->getBeginLoc(),
+         "do not use '%0' with streams; use '\\n' instead")
+        << SourceText << FixItHint::CreateReplacement(TokenRange, "'\\n'");
   } else {
     // Handle the less common function call 'std::endl(...)' case
     const auto *CallExpression = llvm::cast<CallExpr>(Expression);
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8d028f8863cb7a..6768e243add997 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -120,6 +120,10 @@ Changes in existing checks
   <clang-tidy/checks/modernize/use-std-print>` check to support replacing
   member function calls too.
 
+- Improved :doc:`performance-avoid-endl
+  <clang-tidy/checks/performance/avoid-endl>` check by fixing incorrect
+  message.
+
 - Improved :doc:`readablility-implicit-bool-conversion
   <clang-tidy/checks/readability/implicit-bool-conversion>` check
   by adding the option `UseUpperCaseLiteralSuffix` to select the
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/avoid-endl.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/avoid-endl.cpp
index 462b88773d231c..dd2d61a78d7965 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/avoid-endl.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/avoid-endl.cpp
@@ -225,3 +225,14 @@ void bad_custom_stream() {
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]
   // CHECK-FIXES: logger << '\n';
 }
+
+namespace gh107859 {
+
+#define ENDL std::endl;
+
+void bad_macro() {
+  std::cout << ENDL;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use '::std::endl' with streams; use '\n' instead [performance-avoid-endl]
+}
+
+} // namespace gh107859

@HerrCai0907 HerrCai0907 force-pushed the 107859-clang-tidy-broken-diagnostic-for-performance-avoid-endl branch from 65f072d to d7ce4dc Compare September 12, 2024 13:59
Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -123,6 +123,10 @@ Changes in existing checks
<clang-tidy/checks/modernize/use-std-print>` check to support replacing
member function calls too.

- Improved :doc:`performance-avoid-endl
<clang-tidy/checks/performance/avoid-endl>` check to use ``std::endl`` as
placeholder when lexer cannot get source text.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds better IMO

as a default when the lexer can not retrieve the source text.

@HerrCai0907 HerrCai0907 merged commit b7914df into llvm:main Sep 13, 2024
9 checks passed
@HerrCai0907 HerrCai0907 deleted the 107859-clang-tidy-broken-diagnostic-for-performance-avoid-endl branch September 13, 2024 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] Broken diagnostic for performance-avoid-endl
4 participants