erichkeane wrote: > Handles #123121 > > @erichkeane Hi, I’m working on converting `select` statements to > `enum_select` in this draft PR, specifically targeting statements with 5 or > more options. I have a few questions and would appreciate some clarifications:
Awesome, glad to hear it! > > The patch that introduced `enum_select` focused on select statements with 5 > or more options, which is why I’ve prioritized those. Are there any cases > where select statements with fewer than 5 options should also be converted? > Is the number of options alone a determining factor, or does the presence of > magic numbers also need to be considered for conversion? 5 is a pretty decent heuristic for a first-pass. I would expect though that the USES of these are much more important than the count. The point of `enum_select` is to make the call-sites more readable, so this is for situations where there are a LOT of magic-numbers being used in the callers. Places where we have some other logic (besides the magic numbers) to determine the `select` value probably doesn't really qualify here. > By "magic numbers," I understand the patch refers to hardcoded values like /* > ThingIWantSelected */ 5, which lack clarity. Could you provide some concrete > examples from the source code where this applies? Also, I’ve yet to come > across any enum specifically made for a select. Could you clarify when that > is necessary and give an example? > For example: see the ones I changed here: https://github.com/llvm/llvm-project/commit/bf17016a92bc8a23d2cdd2b51355dd4eb5019c68 All/most of the calls/uses of diagnostics there are just hard coded values, which are pretty unreadable/error-prone. > The note about "Ones that are 'calculated' based on some other criteria > aren't a great" isn’t entirely clear to me. Could you elaborate on this? > Sure! Basically "not magic numbers". That is, if the diagnostic-select is always called with some sort of expression that wouldn't be reasonably replaced with the enumerator, it doesn't make sense to change it. BUT if it is always called with an integer constant (or even, passed-to-the-calling function as an integer, even if that integer was derived via logic elsewhere), the enum_select probably makes sense to use. > Lastly, I’d appreciate it if you could review the statements I’ve converted > so far to ensure I’m on the right track. They currently don't seem to build? But the real way I'd review these is based on how much it improves the readability of the call sites, which you haven't changed. https://github.com/llvm/llvm-project/pull/130868 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
