[PATCH] D124500: [clang-tidy] Support expressions of literals in modernize-macro-to-enum

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added a comment.

In D124500#3485462 <https://reviews.llvm.org/D124500#3485462>, @aaron.ballman wrote:

> In D124500#3483328 <https://reviews.llvm.org/D124500#3483328>, @LegalizeAdulthood wrote:
>
>> In D124500#3483224 <https://reviews.llvm.org/D124500#3483224>, @aaron.ballman wrote:
>>
>>> To clarify my previous comments, I'm fine punting on the edge cases until user reports come in, so don't let them block this review if you feel strongly about not supporting them. But when user reports start coming in, at some point I might start asking to replace the custom parser with calling into the clangParse library through some invented utility interface so that we don't have to deal with a long tail of bug reports.
>>
>> Yeah, I think punting on edge cases is the right thing to do here.  As I say,
>> the worst that happens is your macro isn't converted automatically when you
>> could convert it manually.
>
> I largely agree, but I've found cases where we'll convert correct code to incorrect code, so it's a bit stronger than that.

Are you talking generally, or with this check?  I can't see how this check
is going to generate incorrect code (so far).

> I think that's a reasonable goal, but we're not meeting the "never ever generate invalid code" part.

How so?  Can you give an example where this check will produce invalid code?

>> As for calling into `clangParse`, I think that would be overkill for a couple reasons.
>> First, the real parser is going to do a lot of work creating AST nodes which we will
>> never use, except to traverse the structure looking for things that would invalidate
>> it as a candidate for a constant initializing expression.  Second, we only need to
>> match the structure, we don't need to extract any information from the token stream
>> other than a "thumbs up" or "thumbs down" that it is a valid initializing expression.
>
> There's a few reasons I disagree with this. First, you need to know the value of the
> constant expression in order to know whether it's valid as an enumeration constant.

I'm not following you.  Nothing requires knowing this yet.

>   constexpr int a = 12;
>   constexpr int foo() { return 12; }
>   
>   #define FOO (a + 1)
>   #define BAR (a + 2)
>   #define BAZ (a + 3)
>   #define QUUX (foo() + 4)

`QUUX` will never be converted to an enum by this check.  It references an identifier `foo`.

> The situations under which it will break correct code should be something we document explicitly

You haven't shown an example yet where it will break code.


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

https://reviews.llvm.org/D124500