[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#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.

Maybe we're thinking about this check differently.

I want this check to do the majority of the heavy lifting so that I'm only left with a
few "weird" macros that I might have to convert by hand.  I never, ever, ever want
this check to generate invalid code.  Therefore it is of paramount importance that
it be conservative about what it recognizes as a candidate for conversion.

I think this is the baseline for all the modernize checks, really.  There are still cases
where loop-convert doesn't recognize an iterator based loop that could be
converted to a range-based for loop.  The most important thing is that loop-convert
doesn't take my weird iterator based loop and convert it to a range based for loop
that doesn't compile.

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.
Many times in clang-tidy reviews performance concerns are raised and I think
matching the token stream with the recursive descent matcher here is going to be
much, much faster than invoking `clangParse`, particularly since the matcher bails
out early on the first token that doesn't match.  The only thing I can think of that
would make it faster is if we could get the lexed tokens from the preprocessor
instead of making it re-lex the macro body, but that's a change beyond the scope
of this check or even clang-tidy.


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

https://reviews.llvm.org/D124500