LegalizeAdulthood marked 2 inline comments as done. LegalizeAdulthood added inline comments.
================ Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:9 + +// C forbids comma operator in initializing expressions. +#define BAD_OP 1, 2 ---------------- aaron.ballman wrote: > LegalizeAdulthood wrote: > > aaron.ballman wrote: > > > It's also forbidden in C++. > > C++ just talks about a constant expression and doesn't explicitly disallow > > `operator,` in such expressions that I could find. Can you point me to > > where it is disallowed? > > > > If it is disallowed universally, then I don't see why you asked me to parse > > it `:)` > > C++ just talks about a constant expression and doesn't explicitly disallow > > operator, in such expressions that I could find. Can you point me to where > > it is disallowed? > > It falls out from the grammar: > > http://eel.is/c++draft/enum#nt:enumerator-definition > http://eel.is/c++draft/expr.const#nt:constant-expression > http://eel.is/c++draft/expr.cond#nt:conditional-expression > > > If it is disallowed universally, then I don't see why you asked me to parse > > it :) > > It can show up in paren expressions and the interface is generally about > integer literal expressions. OK, yeah, I was looking at the PDF and it doesn't have those nice navigable links, I'll have to keep that site in mind. So.... we have to disallow top-level comma but allow it as a parenthesized expression. Tricky! ================ 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 + ---------------- 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. 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
