fn *intra_pred_dsp_init*: Cleanup and make safe

[kkysen]

No description provided.

[thedataking]

I think it would be wise to delete all uses of bd_fn and similar macros. It optimizes for code compactness instead of readability which I think is the wrong tradeoff. We can simply have:

dsp_init_fn(...)

match bitdepth {
    8 => { init all the 8bpc stuff here }
    16 => { init all the 16bpc stuff here }
} 

This of course makes the code twice as long but more than twice as readable. I don't have to read the bd_fn macro figure out what is going on, I can just see what function is being called directly and I can click on the function name in my IDE to go to the callee. Constructing the function name in a macro is just not worth the mental overhead and hassle IMO.

[kkysen]

I think it would be wise to delete all uses of bd_fn and similar macros. It optimizes for code compactness instead of readability which I think is the wrong tradeoff. We can simply have:

dsp_init_fn(...)

match bitdepth {
    8 => { init all the 8bpc stuff here }
    16 => { init all the 16bpc stuff here }
} 

This of course makes the code twice as long but more than twice as readable. I don't have to read the bd_fn macro figure out what is going on, I can just see what function is being called directly and I can click on the function name in my IDE to go to the callee. Constructing the function name in a macro is just not worth the mental overhead and hassle IMO.

I respectfully disagree, and I feel like we've (including @randomPoison) discussed it before. The bd_fn! way is significantly more readable to me, and C is using a similar macro here, too. If they're all fully expanded, you're never going to notice if one of them is subtly different, but you would if you couldn't use bd_fn! and the difference is glaringly obvious. Hence why I feel it's more readable. It's a consistent, single pattern used throughout the codebase. It's hiding the redundant information that obscures the important information, making it much more readable. And our main focus is safety, performance, and making things safe as fast as possible, not focusing on readability and ergonomics yet. Porting it to use bd_fn! has been much faster for me with much fewer subtle bugs, which I made accidentally more often when I expanded the macro on my initial attempt before adding bd_fn!.

[thedataking]

we've (including @randomPoison) discussed it before

Yes, we struck a reasonable compromise in PR #322 .

Since then, it seems bd_fn and wrap_fn_ptr got added so now the reader has more to parse through to figure out what is happening.

We should not spend our time compactifying the code by wrapping macros in other macros. Readers that aren't you are not going to find it as readable as you do; I'm among them. That said, I acknowledge your point that the C code uses macros here. I'll let @randomPoison decide what is the best way forward.

[rinon]

We've discussed the macros and function pointer wrapping. While there may have been other ways to do this with less complexity or effort, I think that what we have now is usable (now that it's documented) and I don't think we should spend any more time here in either refining or removing the current implementation.

This PR LGTM, the discussion here is not actually relevant for this PR itself, more of a meta-discussion. I would suggest we merge this and the previous changes and move on with what we have here.