aaron.ballman added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:10 +// C forbids comma operator in initializing expressions. +#define BAD_OP 1, 2 + ---------------- LegalizeAdulthood wrote: > aaron.ballman wrote: > > Can you add a test: > > ``` > > #define GOOD_OP (1, 2) > > ``` > > to make sure it still gets converted to an enum? > Yeah, another one I was thinking of is when someone does [[ > https://godbolt.org/z/c677je8s1 | something as disgusting as this. ]] > > ``` > #define ARGS 1, 2 > #define OTHER_ARGS (1, 2) > > int f(int x, int y) { return x + y; } > > int main() > { > return f(ARGS) + f OTHER_ARGS; > } > ``` > > However, the only real way to handle avoiding conversion of the 2nd case is > to examine the context of macro expansion. This is another edge case that > will have to be handled subsequently. > > This gets tricky because AFAIK there is no way to select expressions in the > AST that result from macro expansion. You have to match the macro expansion > locations against AST nodes to identify the node(s) that match the expansion > location yourself. That's a good test case (for some definition of good)! :-) At this point, there's a few options: 1) Go back to the way things were -- disallow comma expressions, even in parens. Document it as a limitation. 2) Accept comma expressions in parens, ignore the problem case like you described above. Document it as a limitation? 3) Try to solve every awful code construct we can ever imagine. Document the check is perfect in every way, but probably not land it for several years. I think I'd be happy with #2 but I could also live with #1 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125622/new/ https://reviews.llvm.org/D125622 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
