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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
