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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
