Hi! On 11/18/21 3:32 PM, Segher Boessenkool wrote: > On Thu, Nov 18, 2021 at 03:30:48PM -0600, Bill Schmidt wrote: >> On 11/18/21 3:16 PM, Segher Boessenkool wrote: >>> Hi! >>> >>> On Wed, Nov 17, 2021 at 05:06:05PM -0600, Bill Schmidt wrote: >>>>> I don't like that at all. The user didn't write the _vsx thing, and it >>>>> isn't documented either (neither is the _vec one, but that is a separate >>>>> issue, specific to this builtin). >>>> I feel like I haven't explained this well. This kind of thing has been in >>>> existence forever even in the old builtins code. The combination of the >>>> error showing the internal builtin name, and the note tying the overload >>>> name to the internal builtin name, has been there all along. The name of >>>> the internal builtin is pretty meaningless. The only thing that's >>>> interesting >>>> in this case is that we previously didn't get this *for this specific case* >>>> because the old code went to a generic fallback. But in many other cases >>>> you get exactly this same kind of error message for the old code. >>> Yes. And it still is a regression (in *this* case). >> Sorry, I don't understand. Why specifically is this a regression? > It is wrong now, in ways that it wasn't wrong before. That is the > definition of regression!
I'm sorry, I disagree. With clarification of the note, I don't see how this can be considered a regression. It is providing information in a different way, but it is still clear that the user has misused the builtin in the context, and, unlike before, it now tells them *what* is wrong with the options that were used (not just "unavailable in this configuration"). The fact that an internal builtin name is *also* disclosed as part of this does not make it wrong. The way that overloads work, we can only tell whether a builtin is enabled with the current set of options by looking at the true builtin that the overload maps to. The enablement checking code doesn't have any knowledge that an overloaded function maps to it. So that error message is produced without knowledge of the overloading. The note diagnostic is added by the overload processing code that *is* aware that the mapping exists. The enablement checking code (rs6000_invalid_builtin in the old code, rs6000_invalid_new_builtin in the new code) is called from multiple places, and not always in an overload context, so we can't assume this is the case. Not all builtins are mapped to by overloads, but they still need enablement checking. Would it be possible to change things so that we pass in the overload name to be used in the error message when appropriate? Yes. But this would have a much larger impact on the test suite than the current method, because all error tests for overloads would now have to change. That is, there are many existing tests that are already "wrong" in the sense that they report the internal builtin name. I suggest that we add that to the list of future cleanups due to the size of the impact. I agree that it has never been ideal to mention the internal builtin name on these messages. It's just not unique to the changes I've made here; it's a pre-existing condition that needs work to cleanse it everywhere. Can we move forward this way? Thanks, Bill > > > Segher
