[Date Prev][Date Next] [Chronological] [Thread] [Top]

RE: (ITS#6505) ldap_search_* functions missing const qualifiers



This is a multi-part message in MIME format.

------_=_NextPart_001_01CAD483.E1DB174F
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

-----Original Message-----
>From: masarati@aero.polimi.it [mailto:masarati@aero.polimi.it]
>In general, upload to ftp.openldap.org according to contribution
>instructions, and post a message with the URL; the way you provided it
>makes it pretty unusable.
  I saw the instructions about using the ftp site some time after I sent =

the patches.  Sorry.  The patch was just the obvious, mechanical change
to add const to the attrs parameter in the functions that have it.

Maybe a reminder to keep reading the rest of the page in the section on
"patches" would be helpful.  I got to the part that started talking =
about
attribution of large changes, and assumed the rest of the page wasn't
relevant to what I was trying to do.

>However, I think your patch will not be considered, although in =
principle
>perfectly acceptable, because an API change of this type would probably
>break a lot of code out there, without significant advantages.

Well, a couple of observations:
 1) At least one other ldap implementation already does this.
 2) I consider being able to turn on the compiler warnings that tell you =

  about parameters that might be changed when do don't expect them to be =

  a significant advantage.  For many of the source trees I've worked on, =

  doing so (and treating warnings as errors, to force them to be fixed)=20
  has helped uncover a variety of bugs.
 3) If you're not being pedantic about warnings, and causing them to be=20
  treated as errors, this API change just results in an extra warning,=20
  which wouldn't actually break any code.
 4) Adding "const" effectively and simply documents the fact that those=20
  parameters aren't going to be changed.  If the API doesn't say so, it
  would be nice if that was at least explicitly mentions in the man =
page.
  At the moment, I had to go read an analyze the ldap code to figure
  that out.  I'd rather spend my time working on my own code instead, =
and
  be able to simply use the great library you guys have created. :)

Anyway, if the change doesn't get accepted it won't be the end of the
 world, but obviously I'm hoping you find the above convincing.

eric

------_=_NextPart_001_01CAD483.E1DB174F
Content-Type: text/html;
	charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV=3D"Content-Type" CONTENT=3D"text/html; =
charset=3Diso-8859-1">
<META NAME=3D"Generator" CONTENT=3D"MS Exchange Server version =
6.5.7654.12">
<TITLE>RE: (ITS#6505) ldap_search_* functions missing const =
qualifiers</TITLE>
</HEAD>
<BODY>
<!-- Converted from text/plain format -->

<P><FONT SIZE=3D2>-----Original Message-----<BR>
&gt;From: masarati@aero.polimi.it [<A =
HREF=3D"mailto:masarati@aero.polimi.it";>mailto:masarati@aero.polimi.it</A=
>]<BR>
&gt;In general, upload to ftp.openldap.org according to contribution<BR>
&gt;instructions, and post a message with the URL; the way you provided =
it<BR>
&gt;makes it pretty unusable.<BR>
&nbsp; I saw the instructions about using the ftp site some time after I =
sent<BR>
the patches.&nbsp; Sorry.&nbsp; The patch was just the obvious, =
mechanical change<BR>
to add const to the attrs parameter in the functions that have it.<BR>
<BR>
Maybe a reminder to keep reading the rest of the page in the section =
on<BR>
&quot;patches&quot; would be helpful.&nbsp; I got to the part that =
started talking about<BR>
attribution of large changes, and assumed the rest of the page =
wasn't<BR>
relevant to what I was trying to do.<BR>
<BR>
&gt;However, I think your patch will not be considered, although in =
principle<BR>
&gt;perfectly acceptable, because an API change of this type would =
probably<BR>
&gt;break a lot of code out there, without significant advantages.<BR>
<BR>
Well, a couple of observations:<BR>
&nbsp;1) At least one other ldap implementation already does this.<BR>
&nbsp;2) I consider being able to turn on the compiler warnings that =
tell you<BR>
&nbsp; about parameters that might be changed when do don't expect them =
to be<BR>
&nbsp; a significant advantage.&nbsp; For many of the source trees I've =
worked on,<BR>
&nbsp; doing so (and treating warnings as errors, to force them to be =
fixed)<BR>
&nbsp; has helped uncover a variety of bugs.<BR>
&nbsp;3) If you're not being pedantic about warnings, and causing them =
to be<BR>
&nbsp; treated as errors, this API change just results in an extra =
warning,<BR>
&nbsp; which wouldn't actually break any code.<BR>
&nbsp;4) Adding &quot;const&quot; effectively and simply documents the =
fact that those<BR>
&nbsp; parameters aren't going to be changed.&nbsp; If the API doesn't =
say so, it<BR>
&nbsp; would be nice if that was at least explicitly mentions in the man =
page.<BR>
&nbsp; At the moment, I had to go read an analyze the ldap code to =
figure<BR>
&nbsp; that out.&nbsp; I'd rather spend my time working on my own code =
instead, and<BR>
&nbsp; be able to simply use the great library you guys have created. =
:)<BR>
<BR>
Anyway, if the change doesn't get accepted it won't be the end of =
the<BR>
&nbsp;world, but obviously I'm hoping you find the above convincing.<BR>
<BR>
eric</FONT>
</P>

</BODY>
</HTML>
------_=_NextPart_001_01CAD483.E1DB174F--