[PATCH] Fix up discard-const warnings in s4 and tries to remove "discard_const_p"s/"CONST DISCARD"s in both s3 and s4

[Matthias Dieter Wallnöfer]

Hi tridge,

tridge at samba.org schrieb:
> Hi Matthias,
>
>  > as suggested by abartlet, I tried to reorganize the patch in smaller 
>  > commits (e.g. const related warnings, non-const related warnings for s4; 
>  > const fixes without functional changes - "discard_const_p" macro, const 
>  > fixes with functional changes for s3...). I hope I made it now easier to 
>  > review (I know it's a very heavy patch).
>
> const warnings are meant as a useful tool for developers, but they do
> not override other programming considerations.
>
> As I've mentioned in the past, the C language makes it close to
> impossible to make large programs that avoid all const warnings. The
> use of discard_const_p() is not evil, as long as it used carefully.
>   
I took as base the comment over the definition of "discard_const_p" in 
"replace.h". There is written that the use should be prevented 
everywhere where it's possible. :)
> In your patch you copy strings where previously we directly used a
> const string. That means the code becomes larger and slower, plus you
> open up the possibility of an allocation failure where we previously
> had none. That is a worse outcome that what we had previously.
>
> You've also made the code much less readable. For example, your change
> in schema_subclasses_recurse() makes this change:
>
> -               struct dsdb_class *schema_class2 = dsdb_class_by_lDAPDisplayName(schema, list[i]);
> +               struct dsdb_class *schema_class2 = discard_const_p(
> +                       struct dsdb_class, dsdb_class_by_lDAPDisplayName(
> +                                                       schema, list[i]));
>
> Wrapping the dsdb_class_by_lDAPDisplayName() call in discard_const_p()
> makes it very hard to see what is going on. For that example I think
> it would be better to first investigate whether that variable can in
> fact be const. Does schema_subclasses_recurse() or the functions it
> calls actually modify the class? If not then it can take a const
> structure pointer. If it does modify it, then you could either provide
> a non-const accessor function for dsdb_class_by_lDAPDisplayName() or
> you could use discard_const_p() but on separate lines of code,
> allowing someone reading the code to understand what is going on.
>   
I know, I know tridge. But you can't image how long I added and removed 
"const"s or added and removed "discard_const_p" macros until I got it to 
compile without warnings. But why e.g. the 
"dsdb_class_bylDAPDisplayName" has to return a "const" result? Couldn't 
we make it non-const? That would us save from some discards.
> When going on a const warning campaign, you need to put considerable
> effort into each case. There is no single solution that fits every
> case, and patches that try to fix const warnings everywhere are doomed
> to failure. You have to understand the intention of each bit of code,
> and make sure you choose the right solution. It would be better to
> leave the warnings in than to put in patches that silence the
> warnings, but make the code worse.
>   
I tried very hard to understand each code part (consider also the error 
handling which I added on each place where it is necessary).
I fixed only strings (since other objects - you know - deep copies are 
harder to create and maybe also very large) which generally aren't that 
large - therefore the additional memory amount shouldn't be too high. I 
tried also to use only temporary contexts If you noticed - since I'm 
aware of the memory handling. Last but not least it's also the question 
how good the "talloc" contexts are freed after use.

Another solution (but yeah - would require much rework and isn't that 
nice) would be to drop the consts from those strings in the parameter lists.

Matthias
> Cheers, Tridge
>
>