• Add C++ test case for acceptable operator, initializer

• Update from review comments
• Disallow operator, unless inside parentheses

Update from review comments

Another observation:
If some new pattern comes up and fixits aren't recognized for a check, it might be better
to switch to a whitelist for checks with fixits rather than going crazier on the file scraping.

LGTM

Are we waiting on some sort of benchmark before accepting/rejecting this change?

If the only thing holding this up is my comments about following the Law of Demeter, I don't think that should block submitting this change.

LGTM

LGTM
Address one nit and ship.

In D125771#3525252, @njames93 wrote:

Tbh the whole testing infrastructure for clang-tidy is a mess. When I have wanted to verify fixes and diagnostics for header files, I find manually invoking clang-tidy is better than using the check_clang_tidy script.
I don't have the time right now, but a verify mode like with clang is something that we should move to in the future.

In D124806#3523228, @njames93 wrote:

In D124806#3523109, @LegalizeAdulthood wrote:

LGTM! Thanks for taking my idea and implementing it much better than what I had :).

Cheers.
Can I ask why you feel that this should be behind an option?

LGTM

Discarding in favor of Nathan James's improved implementation.

LGTM! Thanks for taking my idea and implementing it much better than what I had :).

LGTM

Another good catch!

Good catch!

Just for my own edification, how did you know/suspect that a pure visitor would be faster than matchers?

I thought there wasn't any support for validating fixits applied to header files?

Update documentation from review comments

OK, so thinking about this review a little more, I propose this:

• Take the check as is, but document that the initializing expressions may result in an invalid enum, particularly for C which restricts the underlying type to be int
• Create a subsequent commit that rejects the enums where the language is C and the initializing expression is a value larger than an int by rejecting any macro where any integer token in the expression is larger than an int
• Create an additional subsequent commit that not only matches the expression but also computes the value and checks it for range.

In D124650#3486859, @njames93 wrote:

I've done my own implementation, but its definitely over engineered, WDYT? D124806

In D124500#3485462, @aaron.ballman wrote:

In D124500#3483328, @LegalizeAdulthood wrote:

In D124500#3483224, @aaron.ballman wrote:

To clarify my previous comments, I'm fine punting on the edge cases until user reports come in, so don't let them block this review if you feel strongly about not supporting them. But when user reports start coming in, at some point I might start asking to replace the custom parser with calling into the clangParse library through some invented utility interface so that we don't have to deal with a long tail of bug reports.

Yeah, I think punting on edge cases is the right thing to do here. As I say,
the worst that happens is your macro isn't converted automatically when you
could convert it manually.

I largely agree, but I've found cases where we'll convert correct code to incorrect code, so it's a bit stronger than that.

I also noticed that some of the simplifications being performed resulted in !(x || !y) coming out of other parts of the simplifier, so it should be the case that running this check on your code should result in no further changes to your code if you run the check again.

Testing on VTK revealed this change:

-  this->SliceMapper->SetBackground((this->Background &&
-    !(this->SliceFacesCamera && this->InternalResampleToScreenPixels &&
-      !this->SeparateWindowLevelOperation)));
+  this->SliceMapper->SetBackground(
+      (this->Background &&
+       (!this->SliceFacesCamera && this->InternalResampleToScreenPixels ||
+        this->SeparateWindowLevelOperation)));

Which is incorrect. So more improvement is needed for the case of !(x && y && !z) or !(x || y || !z) is encountered.

FYI, once nice addition of the parsing of macro bodies is that it paves the way for
a modernize-macro-to-function check that converts function-like macros that
compute values to template functions. Once this change has landed, I'll be putting
up a review for that.

Recognize comma operator expressions

Surround replacements with parens

In D124650#3481514, @LegalizeAdulthood wrote:

General question:

There is the question of whether or not the replacement should have ()s around the whole
thing because we don't analyze the context to see if the change in precedence between
operator! and operator&& or operator|| makes any difference.

Update from review comments

In D124500#3483224, @aaron.ballman wrote:

To clarify my previous comments, I'm fine punting on the edge cases until user reports come in, so don't let them block this review if you feel strongly about not supporting them. But when user reports start coming in, at some point I might start asking to replace the custom parser with calling into the clangParse library through some invented utility interface so that we don't have to deal with a long tail of bug reports.

Sort changes by check name in docs

General question:

Update documentation

Add file block comment

Inline Variable

• Add banner block comment for new files
• Extract Functions to eliminate duplication

Update from review comments

Gentle ping.