On Fri Oct 6 07:28:22 2023 +0000, Rémi Bernon wrote:
Why change the logic? I don't think we want to add a joystick if any of these calls fail. Same question with the error levels. I don't know precisely what we consider to deserve ERR vs WARN but I had the impression that ERR is reserved for critical failures that will prevent Wine from working. Most if not all of these failures aren't and are possibly completely acceptable. Fwiw looking at the next commit, I think you could instead just have a `set_joystick_data_format` helper replacing the `IDirectInputDevice8_SetDataFormat` call.
My (re)interpretation of error levels has been that ERR should be for internal errors that a well-behaved application doesn't trigger, whereas WARN is for errors that might happen naturally but might also be symptomatic of a failure. Wine code doesn't always follow this interpretation (although I think most of the more well-maintained subsystems do), but I think it broadly makes sense given what's printed by default. (And usually where Wine code doesn't follow this, it errs on the side of being *more* verbose rather than less.)
In this case the motivation for the change is that almost none of those calls should fail if we're passing valid parameters (and not fatally out of memory). If they do fail, it's an internal error and we should be noisy about it; I'd think an assert would not be unwarranted even. This is also the motivation for changing the control flow, because it's really just not worth the extra code to handle the failure.
The original motivation for this is that the if/else model doesn't work well if we need to do something nontrivial between two method calls. As you say, though, a set-data-format helper would also achieve this, so I can revert this patch and do that instead; I don't feel that strongly about it. (I do still feel that ERR is more appropriate than WARN, though.)