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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/IntegralLiteralExpressionMatcher.cpp:204
+  return true;
+}
+
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > 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.
> "Don't let the perfect be the enemy of the good."
> 
> My inclination is to simply explicitly state that comma operator is not recognized in the documentation.  It's already implicit by it's absence from the list of recognized operators.
> 
> Again, the worst that happens is that your macro isn't converted.
> 
> I'm open to being convinced that it's important, but you haven't convinced me yet `:)`
It wasn't much extra work/code to add comma operator support so I've done that.


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

https://reviews.llvm.org/D124500