LegalizeAdulthood marked 5 inline comments as done.
LegalizeAdulthood added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:324
{
- IntegralLiteralExpressionMatcher Matcher(MacroTokens);
- return Matcher.match();
+ IntegralLiteralExpressionMatcher Matcher(MacroTokens, LangOpts.C99 == 0);
+ bool Matched = Matcher.match();
----------------
aaron.ballman wrote:
> Huh? Comma wasn't allowed in C89 either... In fact, there's no language mode
> which allows top-level commas in either C or C++ -- the issue with the comma
> operator is a parsing one. You can't tell the difference between the comma
> being part of the initializer expression or the comma being the separator
> between enumerators.
The most recent diff handles the issue with `operator,` but the previous
version was only handling the overly-big literal
================
Comment at: clang-tools-extra/clang-tidy/modernize/MacroToEnumCheck.cpp:326
+ bool Matched = Matcher.match();
+ if (LangOpts.C99 &&
+ (Matcher.largestLiteralSize() != LiteralSize::Int &&
----------------
aaron.ballman wrote:
> And C89?
`C99` was the earliest dialect of C I could find in `LangOptions.def`. If you
can show me an earlier version for C, I'm happy to switch it.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize-macro-to-enum.c:3-7
+// C requires enum values to fit into an int.
+#define TOO_BIG1 1L
+#define TOO_BIG2 1UL
+#define TOO_BIG3 1LL
+#define TOO_BIG4 1ULL
----------------
aaron.ballman wrote:
> How do we want to handle the fact that Clang has an extension to allow this?
> Perhaps we want a config option for pedantic vs non-pedantic fixes?
I was trying to find a way to make it fail on gcc/clang on compiler explorer
and I couldn't get it to fail in either compiler....
Where is the extension documented? It appears to be on by default in both
compilers.
================
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:
> 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
In the most recent diff, I'm supporting `operator,` inside parens for C++ and
never allowing it in C.
I plan to do a subsequent enhancement that examines the expansion locations of
macros and rejects any macro whose expansion doesn't encompass a single
expression. In other words, if the tokens in the macro somehow get
incorporated into some syntactic element that isn't completely encased in a
single expression, then reject the macro.
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
