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
