[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 tridge at samba.org
Tue Sep 29 15:31:51 MDT 2009


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.

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.

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.

Cheers, Tridge


More information about the samba-technical mailing list