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