[PATCH] D123479: [clang-tidy] Support parenthesized literals in modernize-macro-to-enum

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123479#3443467 <https://reviews.llvm.org/D123479#3443467>, @LegalizeAdulthood wrote:

> In D123479#3443401 <https://reviews.llvm.org/D123479#3443401>, @aaron.ballman wrote:
>
>> In D123479#3442968 <https://reviews.llvm.org/D123479#3442968>, @LegalizeAdulthood wrote:
>>
>>> In D123479#3442062 <https://reviews.llvm.org/D123479#3442062>, @njames93 wrote:
>>>
>>>> Would it not be better if these parens were stripped in the fixit as they are unnecessary in the enum value decl?
>>
>> To me, that would be ideal, but not strictly necessary (and perhaps doesn't scale super well across all fix-its) because the code is still correct with the parens, just a bit ugly. We have other similar situations with fix-its (like formatting).
>>
>>> I prefer checks to do one thing only in a straightforward manner.
>>>
>>> It's easier to place things before and after the macro expansion rather than modify the contents
>>> of the expansion.
>>>
>>> It doesn't feel like the responsibility of this check is to decide which parentheses are necessary or not.
>>
>> Hmm, I'm more on the fence.
>
> Me too.  I am just not a fan of an automated check doing "other things" to my code
> that aren't advertised in the description.  This is a macro-to-enum check not a redundant-parens
> check.

What puts me on the fence here is -- your check adds the fix-it and it knows that the parens are unnecessary because there can be no order of operations or side effect issues. So yes, you're not writing a redundant parens check; you're writing a macro to enum check that proposes fixes that keep redundant parens when it could be argued that it's reasonable to remove them by not copying them into the fix-it in the first place.

> BTW, many IDEs highlight unnecessary parentheses and have "quick fixes" to remove them,
> ReSharper for C++ is one such IDE add-on that does this.
>
> I'd prefer there to be a readability-redundant-parentheses check that removes unnecessary
> parentheses across the board everywhere, rather than it being a weird side-effect of turning
> my macros into enums.

I tend to come to this conclusion as well. The fix-it is syntactically valid and semantically correct, so it's not wrong to leave it the way it is.


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

https://reviews.llvm.org/D123479