[PATCH] D150872: [clang-tidy] Add support for new TODO style to google-readability-todo

[Sam McCall via Phabricator via cfe-commits]

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