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

[tridge at samba.org]

Hi Matthias,

 > 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. :)

It doesn't quite say that. It says that you should only add more uses
of the macro where you find it really hard to fix const warnings.

That does not mean that fixing const warnings is the top
priority. There are other more important considerations in our code.

 > 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.

right - it's hard to get const stuff right. That doesn't mean we
should just apply any patch that removes the warning. If it is too
hard then we are better off leaving in the 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.

we could, but the const is there to indicate that in normal use
callers should not modify the returned structure. That is because
modifying the structure could break things badly (eg. the binary
search relies on the structure ordering with respect to several of its
members - if you modify an attribute you may break one of the
dsdb_class_by*() functions).

The problem comes while building the structures themselves. For that
we either need discard_const_p() (but please, not as part of the same
C statement as a complex function call!) or we need separate accessor
functions for non-const versions of the structure. If we have separate
accessor functions then we should probably make them static to the
file that they are used in, as we do not want them generally
available.

 > 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.

You're still assuming that getting rid of that const warning is more
important than avoiding memory allocation. What you need to do is see
if the use of the resulting pointer does in fact require the ability
to modify the contents. If it doesn't then look into making the target
pointer const. If that is too hard then either leave the warning there
(maybe someone else will find a better fix) or use discard_const_p().

There are times when copying the string makes sense, especially if you
can't demonstrate that the contents will not be written to. In that
case please add the copy. Looking over your patch I got the impression
that you didn't think carefully about each place, and instead erred on
the side of doing a copy in far too many places.

 > 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.

no, we really don't want the lp_ string functions to return
non-const. A caller modifying one of those would be a disaster, and
very hard to track down.

I also think you should avoid doing code cleanups in the heimdal/
subtree. We import that from the upstream Heimdal tree, and we should
run any proposed changes past Love before we consider them. If we
applied local const cleanups then it would make merging with new
Heimdal releases really hard.

I'd also note that pretty much every developer gets "const fever" at
some point. I've certainly got it a few times. Unless you are prepared
to put in a _lot_ of work (many weeks at least) then you really have
no chance of making Samba const clean. Then if you do succeed you
should know that we will inevitably get more const warnings again
pretty quickly.

If you really do want to take it on, then do it one small piece at a
time, not all in a big patch. We'll have to review each bit to make
sure it's right. You should expect the process to take a long time, as
reviewing const patches is not a top priority for me (it takes a lot
of effort, for a relatively small gain).

Cheers, Tridge