[PATCH] D117522: [clang-tidy] Add modernize-macro-to-enum check

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked 3 inline comments as done.
LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.cpp:67-68
+
+// Undefining a macro invalidates adjacent macros
+// from being considered as an enum.
+#define REMOVED1 1
----------------
LegalizeAdulthood wrote:
> aaron.ballman wrote:
> > What about an #undef that's not adjacent to any macros? e.g.,
> > ```
> > #define FOO 1
> > #define BAR 2
> > #define BAZ 3
> > 
> > int i = 12;
> > 
> > #if defined(FROBBLE)
> > #undef FOO
> > #endif
> > ```
> > I'm worried that perhaps other code elsewhere will be checking `defined(FOO)` perhaps in cases conditionally compiled away, and switching `FOO` to be an enum constant will break other configurations. To be honest, I'm a bit worried about that for all of the transformations here... and I don't know a good way to address that aside from "don't use the check". It'd be interesting to know what kind of false positive rate we have for the fixes if we ran it over a large corpus of code.
> Yeah, the problem arises whenever you make any changes to a header file.  Did you also change all translation units that include the header?  What about conditionally compiled code that was "off" in the translation unit for the automated change?  Currently, we don't have a way of analyzing a group of translation units together for a cohesive change, nor do we have any way of inspecting more deeply into conditionally compiled code.  Addressing those concerns is beyond the scope of this check (or any clang-tidy check) as it involves improvements to the entire infrastructure.
> 
> However, I think it is worth noting in the documentation about possible caveats.  I think the way clang-tidy avoids this problem now is that you have to request fixes and the default mode is to issue warnings and leave it up to the reader as to whether or not they should apply the fixes.
> 
> I believe I already have logic to disqualify any cluster of macros where any one of them are used in a preprocessor condition (that was the last functional change I made to this check).  Looks like I need to extend that slightly to include checking for macros that are `#undef`'ed.
OK, looks like I was already handling this, LOL.  See line 135

```
// Undefining an enum-like macro results in the enum set being dropped.
```


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

https://reviews.llvm.org/D117522