Frank Cusack wrote:

This applies to patches 11-16.

Having these be individual unrelated patches is very annoying. It's very difficult to review because the change in prototype is not in the same change as the change to the functions. So I haven't done a thorough job, but for once I am going to blame the author instead of my own laziness.

This just looks like churn to me. You change the types of vars from char to unsigned char, but then cast them back when they are used.

In at least 2 cases where I caught it, the change in var type is incorrect and you are just making a trivial change without understanding the effects other than to quiet the compiler on your system. Yes, that is my original intention. I clone the branch and compile it, after my compiler show warnings to me, i just submit these patches without a moment's thought and it's too impetuous.

There may be at least one change that was correct, and that would only have been the case if a radius structure member changed its signedness. Because of the difficulty of reviewing the changes I'm not going to try to go back and pick out where I thought that might have been the case.

I have in the past been extremely diligent about not having any compiler warnings so I'm surprised that rlm_otp is generating any noise. There must be one member in a radius struct somewhere that changed and that is filtering down.

In my opinion, a compiler warning may led to a potentional bug, for example, int a = -1; unsigned int b = 1; (a > b) ? printf("yes") : printf("no"); then compiler tell us "warning: comparison between signed and unsigned", if we ignore it, then -1 > 1, that's not expected. so i'm accustomed to fix all warning. thanks for your review! Tinggong