From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 10 Dec 2019 03:07:38 +0300 From: "Dmitry V. Levin" To: ALT Devel discussion list Message-ID: <20191210000737.GD15867@altlinux.org> References: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191208232108.GC30742@altlinux.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="+HP7ph2BbKc20aGI" Content-Disposition: inline In-Reply-To: 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 00:07:38 -0000 Archived-At: List-Archive: List-Post: --+HP7ph2BbKc20aGI Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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->PkgP= ;}; > >> OkState State() const; > >> =20 > >> - void ReMap(void const * const oldMap, void const * const newMap) > >> + void ReMap(void *oldMap, void *newMap) > >=20 > > Is there any particular reason for stripping const here and in other > > similar places? >=20 > Yes, it's needed due to issues emerging from mixing const and non-const= =20 > pointers with new and allegedly more proper way of calculating rebased=20 > pointers. Sorry, I don't find this argument convincing. I have experienced no const issues in my version of this fix. > > [...] > >> @@ -301,7 +302,7 @@ std::experimental::optional DynamicMMa= p::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) > >=20 > > In my patch RebasePointer invocation was after the idxResult check, > > not before the check. >=20 > Theoretically, order here might be important. In practice, it doesn't=20 > matter. We normally try to write code that raises less questions. > > By the way, in this and other similar cases, > > is there any reason for "Pools !=3D oldPools" check? > > Is RebasePointer incapable of handling this, or is it an optimization? > >=20 >=20 > It's just an optimization, it may be removed. OK > > [...] > >> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer= =2Eh > >> 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_base) > >> +{ > >> + return reinterpret_cast(reinterpret_cast(new_base) + (r= einterpret_cast(ptr) - reinterpret_cast(old_base))); > >> +} > >> + > >> +template > >> +static inline const T* RebasePointer(const T *ptr, void *old_base, vo= id *new_base) > >> +{ > >> + return reinterpret_cast(reinterpret_cast(new_base= ) + (reinterpret_cast(ptr) - reinterpret_cast(old_base)= )); > >> +} > >> + > >> +#endif > >=20 > > Do we really need two templates here? >=20 > Yes, second template with const ptr is needed for=20 > rpmListParser::rpmListParser from rpmlistparser.cc. >=20 > Variable SeenPackages has type SeenPackagesType, which is a typedef to=20 > std::set. Thus, elements are 'const char*',=20 > and either it should be const-casted to 'char*', which is ugly, or=20 > const-correctness should be achieved some other way, for example by=20 > getting rid of unimportant const qualifiers like in my changes. >=20 > And first template is needed for every other case with non-const ptr. To be honest, I find my October version of the fix easier to read. 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. OK, I posted my version of the fix. --=20 ldv --+HP7ph2BbKc20aGI Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJd7uHJAAoJEAVFT+BVnCUIpjMP/i3ze2uCMl7fIFt85mdbOrxT wk2nMVIRVP21r2vbb2c63PwhQchFNSmmJ78uP4m1xGByidonJMcGJqK6UaEbPzCo 1ygPuK9xpCSsW3AwMmvl5FqxYmkUUw2x7pqMoKOUwm9eLY1DpLwYnAN/bciXtSTc wtXOlZ24g3EWjde0xOT010d3kWkftb20M7mrmok+mHvHSnQssMGDKRf/JA8RBzHY KtTP6DDZqVuzQlkbHOG/Md1FJSNbyaom/mz1a/6vZArp6d+gbj4VGNALNmMESFiN CCzbHpXjH4KpRR//Kim0zhMM9nBbq72gE/NEKUWmA9WoAEPFY3mR0iW8GryRfZCS Ui3McyZoUPfh7Ky5Vn5pf4idTZdM4pB61jPEOBPvNmlaez2R/B0P/k5PxqGuruzk vZ2JbizUSH/BHxofYyntiuC3QoczpuLH/PzjbW745pSTucYAQ1NappRgg/2ZWB45 Tm+JCamXawupppNykToPklLJZBdGZ4UTZTzGQJS5ChURTzFWpdUHIwx0/OWJCJmw X4aXV0k1rwQTJXsWEKS91styxbS/8/o6VrJzyLtOzh9hZ+/eKvxtjVWvGot3q1UM cKOFURKdlh+WPtlGWoq8Coa07uSYimSGfKNR6okbkpFfRbaWtAWGDLYVoVz/xyeN bgg+15M5WeHVhWnQ9OmM =Aeqy -----END PGP SIGNATURE----- --+HP7ph2BbKc20aGI--