[clang-tidy] Update google todo checker with style guide changes. #165565
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Quentin Khan (qukhan) ChangesThe Google style guide now allows (and recommends) writing TODOs with the following format: // TODO: bug reference - details about what needs to be done.With this change the checker accepts the new style and suggests in in the fix-it hint. The previous style is still accepted. Full diff: https://github.com/llvm/llvm-project/pull/165565.diff 2 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp b/clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
index 8554870287c81..131d0cd14dda9 100644
--- a/clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
@@ -17,28 +17,37 @@ class TodoCommentCheck::TodoCommentHandler : public CommentHandler {
public:
TodoCommentHandler(TodoCommentCheck &Check, std::optional<std::string> User)
: Check(Check), User(User ? *User : "unknown"),
- TodoMatch("^// *TODO *(\\(.*\\))?:?( )?(.*)$") {}
+ TodoMatch("^// *TODO *((\\((.*)\\))?:?( )?|: *(.*) *- *)?(.*)$") {}
bool HandleComment(Preprocessor &PP, SourceRange Range) override {
StringRef Text =
Lexer::getSourceText(CharSourceRange::getCharRange(Range),
PP.getSourceManager(), PP.getLangOpts());
- SmallVector<StringRef, 4> Matches;
+ SmallVector<StringRef, 7> Matches;
if (!TodoMatch.match(Text, &Matches))
return false;
- StringRef Username = Matches[1];
- StringRef Comment = Matches[3];
+ const bool deprecated_style = !Matches[3].empty();
+ StringRef Username = deprecated_style ? Matches[5] : Matches[3];
+ StringRef Comment = Matches[6];
- if (!Username.empty())
+ if (!Username.empty() && (deprecated_style || !Comment.empty()))
return false;
- std::string NewText = ("// TODO(" + Twine(User) + "): " + Comment).str();
+ if (Username.empty()) {
+ std::string NewText = ("// TODO: " + Twine(User) + " - " +
+ (Comment.empty() ? "some details" : Comment))
+ .str();
+
+ Check.diag(Range.getBegin(), "missing username/bug in TODO")
+ << FixItHint::CreateReplacement(CharSourceRange::getCharRange(Range),
+ NewText);
+ }
+
+ if (Comment.empty())
+ Check.diag(Range.getBegin(), "missing details in TODO");
- Check.diag(Range.getBegin(), "missing username/bug in TODO")
- << FixItHint::CreateReplacement(CharSourceRange::getCharRange(Range),
- NewText);
return false;
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/google/readability-todo.cpp b/clang-tools-extra/test/clang-tidy/checkers/google/readability-todo.cpp
index 6b900aa92150e..5701b30bef395 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/google/readability-todo.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/google/readability-todo.cpp
@@ -2,22 +2,36 @@
// TODOfix this1
// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing username/bug in TODO
-// CHECK-FIXES: // TODO(some user): fix this1
+// CHECK-FIXES: // TODO: some user - fix this1
// TODO fix this2
// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing username/bug in TODO
-// CHECK-FIXES: // TODO(some user): fix this2
+// CHECK-FIXES: // TODO: some user - fix this2
// TODO fix this3
// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing username/bug in TODO
-// CHECK-FIXES: // TODO(some user): fix this3
+// CHECK-FIXES: // TODO: some user - fix this3
// TODO: fix this4
// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing username/bug in TODO
-// CHECK-FIXES: // TODO(some user): fix this4
+// CHECK-FIXES: // TODO: some user - fix this4
+
+// TODO: bug 12345 -
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing details in TODO
+
+// TODO: a message without a reference
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: missing username/bug in TODO
+// CHECK-FIXES: // TODO: some user - a message without a reference
// TODO(clang)fix this5
+// TODO: foo - shave yaks
+// TODO:foo - no space bewteen semicolon and username
+// TODO: foo- no space bewteen username and dash
+// TODO: foo - extra spaces between semicolon and username
+// TODO: foo - extra spaces between username and dash
+// TODO: b/12345 - use a b/ prefix
+// TODO: bug 12345 - use a space in username/bug reference
// TODO(foo):shave yaks
// TODO(bar):
// TODO(foo): paint bikeshed
|
|
@HerrCai0907, I'm pinging you per the docs as I'm a new contributor and I can't manually add reviewers to a PR. |
vbvictor
requested review from
HerrCai0907,
carlosgalvezp,
localspook and
vbvictor
|
✅ With the latest revision this PR passed the C/C++ code linter. |
There was a problem hiding this comment.
Please add release notes.
Could we make option in what style should fixes work?
I didn't find in google docs that TODO(...) is considered deprecated and I personally would like to get fixes in older format other than new TODO: ... - format.
To keep codebases consistent, we should provide a way to choose fixes style.
Done.
Done. I added the
Unfortunately I haven't found any other publicly available information about that. The formats in the style guide are in order of preference. |
qukhan
force-pushed
the
google-todo
branch
2 times, most recently
from
fee5d3c to
791e519
Compare
qukhan
force-pushed
the
google-todo
branch
2 times, most recently
from
f84ae1e to
0f3b462
Compare
The [Google style guide] now allows (and recommends) writing TODOs with the following format: ```cpp // TODO: bug reference - details about what needs to be done. ``` With this change the checker accepts the new style and suggests in in the fix-it hint. The previous style is still accepted. A new configuration option, `google-readability-todo.Style` is available to switch between the `Parentheses` (legacy) style and the `Hyphen` style. [Google style guide]: https://google.github.io/styleguide/cppguide.html#TODO_Comments
4 participants
The Google style guide now allows (and recommends) writing TODOs with the following format:
// TODO: bug reference - details about what needs to be done.With this change the checker accepts the new style and suggests in in the fix-it hint. The previous style is still accepted.