From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 10 Dec 2019 13:20:10 +0300 From: "Dmitry V. Levin" To: ALT Devel discussion list Message-ID: <20191210102009.GB22650@altlinux.org> References: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191208232108.GC30742@altlinux.org> <20191210000737.GD15867@altlinux.org> <1ee850d3-9ebd-ae95-2665-f5ba7fb86ad4@altlinux.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="OgqxwSJOaUobr8KG" Content-Disposition: inline In-Reply-To: <1ee850d3-9ebd-ae95-2665-f5ba7fb86ad4@altlinux.org> Subject: Re: [devel] [PATCH for apt 2/2 v2] Fix pointer arithmetics X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Dec 2019 10:20:10 -0000 Archived-At: List-Archive: List-Post: --OgqxwSJOaUobr8KG Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 10, 2019 at 11:18:06AM +0300, Aleksei Nikiforov wrote: > 10.12.2019 3:07, Dmitry V. Levin =D0=C9=DB=C5=D4: > > On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote: > >> 09.12.2019 2:21, Dmitry V. Levin =D0=C9=DB=C5=D4: > >>> On Fri, Dec 06, 2019 at 06:36:55PM +0300, Aleksei Nikiforov wrote: > >>> [...] > >>>> @@ -85,11 +87,11 @@ class pkgCache::PkgIterator > >>>> inline unsigned long long Index() const {return Pkg - Owner->P= kgP;}; > >>>> OkState State() const; > >>>> =20 > >>>> - void ReMap(void const * const oldMap, void const * const newMap) > >>>> + void ReMap(void *oldMap, void *newMap) > >>> > >>> Is there any particular reason for stripping const here and in other > >>> similar places? > >> > >> Yes, it's needed due to issues emerging from mixing const and non-const > >> pointers with new and allegedly more proper way of calculating rebased > >> pointers. > >=20 > > Sorry, I don't find this argument convincing. > > I have experienced no const issues in my version of this fix. >=20 > Your version is using C-style casts in C++ code. Of course, I could use= =20 > C-style casts or const_cast-s too to work around const correctness=20 > issues (i.e. to just hide these issues), and it'd work like your=20 > version. But I'd like to remind you that APT is C++ project, not a C=20 > project. What might be ok for C is sometimes a dirty ugly hack in modern= =20 > C++. Sorry, I don't share you point of view on this matter. Being a C++ project allows you to use C++ constructs, that's true, but why do you think it prevents you from using C constructs when appropriate? > >>>> @@ -301,7 +302,7 @@ std::experimental::optional DynamicM= Map::Allocate(unsigned long Item > >>>> Pool* oldPools =3D Pools; > >>>> auto idxResult =3D RawAllocate(I->Count*ItemSize,ItemSize); > >>>> if (Pools !=3D oldPools) > >>>> - I +=3D Pools - oldPools; > >>>> + I =3D RebasePointer(I, oldPools, Pools); > >>>> =20 > >>>> // Does the allocation failed ? > >>>> if (!idxResult) > >>> > >>> In my patch RebasePointer invocation was after the idxResult check, > >>> not before the check. > >> > >> Theoretically, order here might be important. In practice, it doesn't > >> matter. > >=20 > > We normally try to write code that raises less questions. >=20 > In that case it's better to keep order from my patch, isn't it?=20 > Practically it's fine either way, but theoretically that order is superio= r. The order in question was introduced by your commit 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes"). If I was reviewing that commit, this would have been fixed already. > >>> [...] > >>>> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_point= er.h > >>>> new file mode 100644 > >>>> index 0000000..f6b3c15 > >>>> --- /dev/null > >>>> +++ b/apt/apt-pkg/rebase_pointer.h > >>>> @@ -0,0 +1,16 @@ > >>>> +#ifndef PKGLIB_REBASE_POINTER_H > >>>> +#define PKGLIB_REBASE_POINTER_H > >>>> + > >>>> +template > >>>> +static inline T* RebasePointer(T *ptr, void *old_base, void *new_ba= se) > >>>> +{ > >>>> + return reinterpret_cast(reinterpret_cast(new_base) + = (reinterpret_cast(ptr) - reinterpret_cast(old_base))); > >>>> +} > >>>> + > >>>> +template > >>>> +static inline const T* RebasePointer(const T *ptr, void *old_base, = void *new_base) > >>>> +{ > >>>> + return reinterpret_cast(reinterpret_cast(new_ba= se) + (reinterpret_cast(ptr) - reinterpret_cast(old_bas= e))); > >>>> +} > >>>> + > >>>> +#endif > >>> > >>> Do we really need two templates here? > >> > >> Yes, second template with const ptr is needed for > >> rpmListParser::rpmListParser from rpmlistparser.cc. > >> > >> Variable SeenPackages has type SeenPackagesType, which is a typedef to > >> std::set. Thus, elements are 'const char*', > >> and either it should be const-casted to 'char*', which is ugly, or > >> const-correctness should be achieved some other way, for example by > >> getting rid of unimportant const qualifiers like in my changes. > >> > >> And first template is needed for every other case with non-const ptr. > >=20 > > To be honest, I find my October version of the fix easier to read. > >=20 > > Since all users of RebasePointer except rpmListParser use it in a form = of > > ptr =3D RebasePointer(ptr, old_base, new_base); > > I find it more natural when RebasePointer updates the pointer, > > so one can write > > RebasePointer(ptr, old_base, new_base); > > instead. > >=20 > > OK, I posted my version of the fix. >=20 > And it's opposite for me. I prefer to explicitly see when variable is=20 > changed. And for all calls it looks exactly same: no matter how it's=20 > used, new pointer is returned from function as a result of function.=20 > Interface uniformity, obviousness and predictability is important. What I don't like in your approach is that it's error-prone: it's very easy to forget the assignment or to assign the result to a wrong variable. In fact, I had to use the following regular expression just to check whether all uses of RebasePointer are correct in that respect: $ git grep -Fw RebasePointer |grep -v '\<\([[:alpha:]][][[:alnum:]_]*\) =3D= RebasePointer(\1,' This is surely not the way how things should be done, neither in C nor in C++. --=20 ldv --OgqxwSJOaUobr8KG Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJd73FZAAoJEAVFT+BVnCUI4VQP/3q2lfpggDQEdwYdCkCCLWQe c3YfjU9YEz4a+fgN58Nw5dCztkZKaShWY0cAM+byl2nyQsKtP5WimETQkAkAx1vH XEfwSlWGr4Y6S5L8W7FBe4BgH3DT9SdO8R54cTVTahXEGoijPsgrw//1kF0sp8pV fDoe3tpv9Bnzr6nAfYHzMKYb8GjQkRX63Vkm46r7nmrFfSVlhrkrwAIzWwbvIfgl aRgnOFQueLr108BvQhkij2aNOtgejRccqa6TTTVsdMeR/LKw0P80oXrle+PL8J8y s+mF9jweJdsQGTC+zVhTXqjYMupTsJzS4CBC7v10/LPpzn1fYEad/ikPvCZ6tePB y167SD2BjHLTPN3B4Zs2sjw/jFurthfBJZbIO0/KFuQWH667iMdGhpYu029e3V8p bdjm4tiOpu6cScEbawt5RZsKFlrSfPs52C423Ys3ve4DADTHTODJWPHIWSX3uWjh vnzEm5n3gxWiRQwIj1aONXoZXqhlKFnek5TuyvZrY3djDxwVX7gzliOzLRMSmwzg NgmhGx+xImMY0Ct9fiVVRDHxOThl/6jlkni8XQby/NrAnnSpWokOhX05oHdIAdyc d7gxTZvAH1NtRDBr+h723D9z2MXo70pjo4kVJJnaWf8Cq+389GOXujHBIa6lDFNb IJDVZdf08n7XYVsU2Gm9 =cwUS -----END PGP SIGNATURE----- --OgqxwSJOaUobr8KG--