ymandel updated this revision to Diff 427126.
ymandel added a comment.
address reviewer comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124952/new/
https://reviews.llvm.org/D124952
Files:
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -90,6 +90,27 @@
EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
}
+TEST(TransformerClangTidyCheckTest, DiagnosticMessageEscaped) {
+ class GiveDiagWithPercentSymbol : public TransformerClangTidyCheck {
+ public:
+ GiveDiagWithPercentSymbol(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(makeRule(returnStmt(),
+ noopEdit(node(RootID)),
+ cat("bad code: x % y % z")),
+ Name, Context) {}
+ };
+ std::string Input = "int somecode() { return 0; }";
+ std::vector<ClangTidyError> Errors;
+ EXPECT_EQ(Input,
+ test::runCheckOnCode<GiveDiagWithPercentSymbol>(Input, &Errors));
+ ASSERT_EQ(Errors.size(), 1U);
+ // The message stored in this field shouldn't include escaped percent signs,
+ // because the diagnostic printer should have _unescaped_ them when
processing
+ // the diagnostic. The only behavior observable/verifiable by the test is
that
+ // the presence of the '%' doesn't crash Clang.
+ EXPECT_EQ(Errors[0].Message.Message, "bad code: x % y % z");
+}
+
class IntLitCheck : public TransformerClangTidyCheck {
public:
IntLitCheck(StringRef Name, ClangTidyContext *Context)
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -27,6 +27,33 @@
" explicitly provide an empty explanation if none is desired");
}
+// If a string unintentionally containing '%' is passed as a diagnostic, Clang
+// will claim the string is ill-formed and assert-fail. This function escapes
+// such strings so they can be safely used in diagnostics.
+std::string escapeForDiagnostic(std::string ToEscape) {
+ // Optimize for the common case that the string does not contain `%` at the
+ // cost of an extra scan over the string in the slow case.
+ auto Pos = ToEscape.find('%');
+ if (Pos == ToEscape.npos)
+ return ToEscape;
+
+ std::string Result;
+ Result.reserve(ToEscape.size());
+ // Convert position to a count.
+ ++Pos;
+ Result.append(ToEscape, 0, Pos);
+ Result += '%';
+
+ for (auto N = ToEscape.size(); Pos < N; ++Pos) {
+ const char C = ToEscape.at(Pos);
+ Result += C;
+ if (C == '%')
+ Result += '%';
+ }
+
+ return Result;
+}
+
TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -99,7 +126,8 @@
}
// Associate the diagnostic with the location of the first change.
- DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
+ DiagnosticBuilder Diag =
+ diag((*Edits)[0].Range.getBegin(), escapeForDiagnostic(*Explanation));
for (const auto &T : *Edits)
switch (T.Kind) {
case transformer::EditKind::Range:
Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -90,6 +90,27 @@
EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
}
+TEST(TransformerClangTidyCheckTest, DiagnosticMessageEscaped) {
+ class GiveDiagWithPercentSymbol : public TransformerClangTidyCheck {
+ public:
+ GiveDiagWithPercentSymbol(StringRef Name, ClangTidyContext *Context)
+ : TransformerClangTidyCheck(makeRule(returnStmt(),
+ noopEdit(node(RootID)),
+ cat("bad code: x % y % z")),
+ Name, Context) {}
+ };
+ std::string Input = "int somecode() { return 0; }";
+ std::vector<ClangTidyError> Errors;
+ EXPECT_EQ(Input,
+ test::runCheckOnCode<GiveDiagWithPercentSymbol>(Input, &Errors));
+ ASSERT_EQ(Errors.size(), 1U);
+ // The message stored in this field shouldn't include escaped percent signs,
+ // because the diagnostic printer should have _unescaped_ them when processing
+ // the diagnostic. The only behavior observable/verifiable by the test is that
+ // the presence of the '%' doesn't crash Clang.
+ EXPECT_EQ(Errors[0].Message.Message, "bad code: x % y % z");
+}
+
class IntLitCheck : public TransformerClangTidyCheck {
public:
IntLitCheck(StringRef Name, ClangTidyContext *Context)
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -27,6 +27,33 @@
" explicitly provide an empty explanation if none is desired");
}
+// If a string unintentionally containing '%' is passed as a diagnostic, Clang
+// will claim the string is ill-formed and assert-fail. This function escapes
+// such strings so they can be safely used in diagnostics.
+std::string escapeForDiagnostic(std::string ToEscape) {
+ // Optimize for the common case that the string does not contain `%` at the
+ // cost of an extra scan over the string in the slow case.
+ auto Pos = ToEscape.find('%');
+ if (Pos == ToEscape.npos)
+ return ToEscape;
+
+ std::string Result;
+ Result.reserve(ToEscape.size());
+ // Convert position to a count.
+ ++Pos;
+ Result.append(ToEscape, 0, Pos);
+ Result += '%';
+
+ for (auto N = ToEscape.size(); Pos < N; ++Pos) {
+ const char C = ToEscape.at(Pos);
+ Result += C;
+ if (C == '%')
+ Result += '%';
+ }
+
+ return Result;
+}
+
TransformerClangTidyCheck::TransformerClangTidyCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
@@ -99,7 +126,8 @@
}
// Associate the diagnostic with the location of the first change.
- DiagnosticBuilder Diag = diag((*Edits)[0].Range.getBegin(), *Explanation);
+ DiagnosticBuilder Diag =
+ diag((*Edits)[0].Range.getBegin(), escapeForDiagnostic(*Explanation));
for (const auto &T : *Edits)
switch (T.Kind) {
case transformer::EditKind::Range:
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
