[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo
sammccall added a comment.
I'd be more comfortable reviewing this after it's actually been added to the
style guide, it's hard to refer to non-public docs and discussions here.
I don't really understand the rationale for the hard split into two modes, vs
always accepting both styles and maybe just making the fixit style configurable.
(OTOH If we *really* want to support exclusive use of the old style, why are we
not supporting exclusive use of the new style?)
Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:18
+enum class TodoStyleVersion {
+ V1,
I'd really prefer if we could give these names rather than numbers. Best I can
come up with is paren-style/hyphen style though.
Either way, we can drop "version" as it's redundant with "style".
(Unless you're using it to refer to the version of the style *guide*/check
logic in which case it's confusing to also use it to refer to the style of the
comment, as these aren't 1:1)
Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:79
+
+if (/* New style*/ (!Matches[2].empty() && !Matches[3].empty()) ||
+/* Old style */ !Matches[4].empty())
I find this part very confusing. We have two separate implementations that
check whether this is a valid `TODO(context): description` comment, even though
that's acceptable in all modes. Why?
It seems like the logic should look either like:
```
if (!isAnyTODOComment) // regex #1
return;
if (isValidParenStyleTODO) // regex #2
return;
if (allowNewStyle && isValidHyphenStyleTODO) // regex #3
return;
diag(...) << Fix; // suggestion based on preferred style and parse from
isAnyTODOComment
```
or
```
if (!matchesComplicatedAllEncompassingRegex)
return;
if (submatchesIndicateValidParenStyleTODO)
return;
if (submatchesIndicateValidHyphenStyleTODO)
return;
// diagnose and suggest fix
```
Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:85
+// style) or it doesn't match any style, beyond starting with TODO.
+Check.diag(Range.getBegin(), "TODO requires link and description");
}
this isn't accurate: `TODO(username): description` is still valid.
Maybe "should have"?
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst:16
+
+.. option:: UseV2Style
+
This is a confusing option name.
It's not clear what "V2Style" is, it's non-descriptive and the style guide
won't use that term.
It's not clear what "Use" means - in fixes? Only accept? My code uses V2 style?
Based on the current behavior, I'd suggest "PreferHyphenStyle"
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst:32
+ It will still admit the previous style, for backwards compatability. But, it
+ won't suggest fixes, since mistakes are now ambiguous: given `TODO: text`,
"text"
+ could be the description or it could be a link of some form.
I think trying to enforce a new style via a check that doesn't suggest or even
describe the style is a mistake.
Given `TODO: text`, `text` is almost certainly a description - creating an
appropriate link is a chore, but IME ~nobody forgets to actually write what
they want to do.
If the suggestion is wrong, people can fix it. The most important thing is we
tell them what the expected format is.
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp:21
+
+// TODO: https://github.com/llvm/llvm-project/issues/12345 - Some description
+// TODO: link - description
(surely this example shows how absurd this style is...)
Comment at:
clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp:24
+
+// V1-style TODO comments, supported for backwards compatability:
+
"supported for backwards compatibility" is stronger than the text I've seen,
which simply says they're discouraged in new code. But for example if you don't
have a bug link, AFAICS `TODO(username): ` is the only possible form.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150872/new/
https://reviews.llvm.org/D150872
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo
ymandel marked 3 inline comments as done.
ymandel added a comment.
Thank you for the very fast review!
Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:97
: ClangTidyCheck(Name, Context),
+ UseV2Style(Options.getLocalOrGlobal("UseV2Style", false)),
Handler(std::make_unique(
njames93 wrote:
> Given this check is part of the google module, doesn't it make sense to have
> the default as what google now recommends.
The style guide hasn't been updated yet. Once that happens, I think we should
switch the default. I can add a FIXME -- what do you think?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150872/new/
https://reviews.llvm.org/D150872
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo
ymandel updated this revision to Diff 523440.
ymandel added a comment.
Address reviewer comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150872/new/
https://reviews.llvm.org/D150872
Files:
clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s google-readability-todo %t -- \
+// RUN: -config="{User: 'some user', CheckOptions: \
+// RUN: [{key: google-readability-todo.UseV2Style, \
+// RUN: value: 'true'}]}" \
+// RUN: --
+
+// TODOfix this1
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO fix this2
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO fix this3
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO: fix this4
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// V2-style TODO comments:
+
+// TODO: https://github.com/llvm/llvm-project/issues/12345 - Some description
+// TODO: link - description
+
+// V1-style TODO comments, supported for backwards compatability:
+
+// TODO(clang)fix this5
+
+// TODO(foo):shave yaks
+// TODO(bar):
+// TODO(foo): paint bikeshed
+// TODO(b/12345): find the holy grail
+// TODO (b/12345): allow spaces before parentheses
+// TODO(asdf) allow missing semicolon
Index: clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
+++ clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
@@ -3,9 +3,32 @@
google-readability-todo
===
-Finds TODO comments without a username or bug number.
+Finds TODO comments not conforming to Google's style guidelines.
The relevant style guide section is
https://google.github.io/styleguide/cppguide.html#TODO_Comments.
Corresponding cpplint.py check: `readability/todo`
+
+Options
+---
+
+.. option:: UseV2Style
+
+ Disabled by default.
+
+ When disabled, the check enforces the style
+ ```
+ // TODO(username/bug): description
+ ```
+ and will suggest fixes in this style, where possible (for example, a TODO
+ missing the username).
+
+ When enabled, the check also accepts TODO comments in the following style:
+ ```
+ // TODO: link - description
+ ```
+ It will still admit the previous style, for backwards compatability. But, it
+ won't suggest fixes, since mistakes are now ambiguous: given `TODO: text`, "text"
+ could be the description or it could be a link of some form.
+
Index: clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
===
--- clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
+++ clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
@@ -27,8 +27,13 @@
void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+ void storeOptions(ClangTidyOptions::OptionMap ) override {
+Options.store(Opts, "UseV2Style", UseV2Style);
+ }
+
private:
class TodoCommentHandler;
+ bool UseV2Style;
std::unique_ptr Handler;
};
Index: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
@@ -7,51 +7,97 @@
//===--===//
#include "TodoCommentCheck.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
+#include "llvm/Support/ErrorHandling.h"
#include
namespace clang::tidy::google::readability {
+enum class TodoStyleVersion {
+ V1,
+ V2,
+};
+
+std::string_view getVersionRegex(TodoStyleVersion V) {
+ switch (V) {
+ case TodoStyleVersion::V1:
+return "^// *TODO *(\\(.*\\))?:?( )?(.*)$";
+ case TodoStyleVersion::V2:
+return "^// *TODO *((: *[^ ]+( - .)?)|([(][^)]*[)])?).*$";
+ }
+ llvm_unreachable("bad enum value");
+}
+
class TodoCommentCheck::TodoCommentHandler : public CommentHandler {
public:
- TodoCommentHandler(TodoCommentCheck , std::optional User)
- : Check(Check), User(User ? *User : "unknown"),
-TodoMatch("^// *TODO *(\\(.*\\))?:?( )?(.*)$") {}
+ TodoCommentHandler(TodoCommentCheck ,
[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo
njames93 added inline comments.
Comment at: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp:97
: ClangTidyCheck(Name, Context),
+ UseV2Style(Options.getLocalOrGlobal("UseV2Style", false)),
Handler(std::make_unique(
Given this check is part of the google module, doesn't it make sense to have
the default as what google now recommends.
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst:6
Finds TODO comments without a username or bug number.
Maybe update this line to something along with lines of
`Finds TODO comments not conforming to googles style guidelines`
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst:16-18
+.. option:: User
+
+ The username to use when suggesting an edit to a TODO comment. Only
relevant to V1 style.
This shouldn't appear in the options, as it isn't read from the `CheckOptions`
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst:22
+
+ Whether to check TODO comments against the new TODO style, specifically:
+ ```
It would be good to specify the default value of this option(or just the
default behaviour of the check)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150872/new/
https://reviews.llvm.org/D150872
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo
ymandel created this revision.
ymandel added a reviewer: sammccall.
Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun.
Herald added a reviewer: njames93.
Herald added a project: All.
ymandel requested review of this revision.
Herald added a project: clang-tools-extra.
Google style is changing to encourage a different (and more common) form of
TODO, specifically: `TODO: link - description`. This patch adds support for the
new style, while maintaining backwards compatability with the old style. The
patch adds a configuration option to the check to control which style (V1 or V2)
is checked.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D150872
Files:
clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/google/readability-todo-v2.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s google-readability-todo %t -- \
+// RUN: -config="{User: 'some user', CheckOptions: \
+// RUN: [{key: google-readability-todo.UseV2Style, \
+// RUN: value: 'true'}]}" \
+// RUN: --
+
+// TODOfix this1
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO fix this2
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO fix this3
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// TODO: fix this4
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: TODO requires link and description
+
+// V2-style TODO comments:
+
+// TODO: https://github.com/llvm/llvm-project/issues/12345 - Some description
+// TODO: link - description
+
+// V1-style TODO comments, supported for backwards compatability:
+
+// TODO(clang)fix this5
+
+// TODO(foo):shave yaks
+// TODO(bar):
+// TODO(foo): paint bikeshed
+// TODO(b/12345): find the holy grail
+// TODO (b/12345): allow spaces before parentheses
+// TODO(asdf) allow missing semicolon
Index: clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
+++ clang-tools-extra/docs/clang-tidy/checks/google/readability-todo.rst
@@ -9,3 +9,17 @@
https://google.github.io/styleguide/cppguide.html#TODO_Comments.
Corresponding cpplint.py check: `readability/todo`
+
+Options
+---
+
+.. option:: User
+
+ The username to use when suggesting an edit to a TODO comment. Only relevant to V1 style.
+
+.. option:: UseV2Style
+
+ Whether to check TODO comments against the new TODO style, specifically:
+ ```
+ // TODO: link - description
+ ```
Index: clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
===
--- clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
+++ clang-tools-extra/clang-tidy/google/TodoCommentCheck.h
@@ -27,8 +27,13 @@
void registerPPCallbacks(const SourceManager , Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
+ void storeOptions(ClangTidyOptions::OptionMap ) override {
+Options.store(Opts, "UseV2Style", UseV2Style);
+ }
+
private:
class TodoCommentHandler;
+ bool UseV2Style;
std::unique_ptr Handler;
};
Index: clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/google/TodoCommentCheck.cpp
@@ -7,51 +7,97 @@
//===--===//
#include "TodoCommentCheck.h"
+#include "clang/Basic/Diagnostic.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
+#include "llvm/Support/ErrorHandling.h"
#include
namespace clang::tidy::google::readability {
+enum class TodoStyleVersion {
+ V1,
+ V2,
+};
+
+std::string_view getVersionRegex(TodoStyleVersion V) {
+ switch (V) {
+ case TodoStyleVersion::V1:
+return "^// *TODO *(\\(.*\\))?:?( )?(.*)$";
+ case TodoStyleVersion::V2:
+return "^// *TODO *((: *[^ ]+( - .)?)|([(][^)]*[)])?).*$";
+ }
+ llvm_unreachable("bad enum value");
+}
+
class TodoCommentCheck::TodoCommentHandler : public CommentHandler {
public:
- TodoCommentHandler(TodoCommentCheck , std::optional User)
- : Check(Check), User(User ? *User : "unknown"),
-TodoMatch("^// *TODO *(\\(.*\\))?:?( )?(.*)$") {}
+ TodoCommentHandler(TodoCommentCheck , std::optional User,
+ TodoStyleVersion V)
+ : Check(Check), User(User ? *User : "unknown"), Version(V),
+
