[PATCH] D124500: [clang-tidy] Support expressions of literals in modernize-macro-to-enum

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:99
+
+  if (!Current->isLiteral() || isStringLiteral(Current->getKind()) ||
+      !isIntegralConstant(*Current)) {
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > I know this is code moved from elsewhere, but I suppose we never considered the odd edge case where a user does something like `"foo"[0]` as a really awful integer constant. :-D
> It's always possible to write crazy contorted code and have a check not recognize it.  I don't think it's worthwhile to spend time trying to handle torture cases unless we can find data that shows it's prevalent in real world code.
> 
> If I was doing a code review and saw this:
> ```
> enum {
>     FOO = "foo"[0]
> };
> ```
> I'd flag that in a code review as bogus, whereas if I saw:
> ```
> enum {
>     FOO = 'f'
> };
> ```
> That would be acceptable, which is why character literals are accepted as an integral literal initializer for an enum in this check.
>  I don't think it's worthwhile to spend time trying to handle torture cases unless we can find data that shows it's prevalent in real world code.

I think I'm okay agreeing to that in this particular case, but this is more to point out that writing your own parser is a maintenance burden. Users will hit cases we've both forgotten about here, they'll file a bug, then someone has to deal with it. It's very hard to justify to users "we think you write silly code" because they often have creative ways in which their code is not actually so silly, especially when we support "most" valid expressions.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:185
+
+  if (Current->is(tok::TokenKind::question)) {
+    if (!advance())
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > There is GNU extension in this space: https://godbolt.org/z/PrWY3T6hY
> Do you have a link for the extension?
https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:204
+  return true;
+}
+
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > Comma operator?
> Remember that the use case here is identifying expressions that are initializers for an enum.  If you were doing a code review and you saw this:
> ```
> enum {
>     FOO = (2, 3)
> };
> ```
> Would you be OK with that?  I wouldn't.  Clang even warns about it: https://godbolt.org/z/Y641cb8Y9
> 
> Therefore I deliberately left comma operator out of the grammar.
This is another case where I think you're predicting that users won't be using the full expressivity of the language and we'll get bug reports later. Again, in insolation, I tend to agree that I wouldn't be happy seeing that code. However, users write some very creative code and there's no technical reason why we can't or shouldn't handle comma operators.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124500/new/

https://reviews.llvm.org/D124500