From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on sa.local.altlinux.org X-Spam-Level: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=unavailable autolearn_force=no version=3.4.1 To: devel@lists.altlinux.org References: <20191206133647.dculnmwkd3yf2wjp@titan.localdomain> <20191206153655.86334-1-darktemplar@altlinux.org> <20191208232108.GC30742@altlinux.org> <20191210000737.GD15867@altlinux.org> From: Aleksei Nikiforov Message-ID: <1ee850d3-9ebd-ae95-2665-f5ba7fb86ad4@altlinux.org> Date: Tue, 10 Dec 2019 11:18:06 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20191210000737.GD15867@altlinux.org> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Language: ru Content-Transfer-Encoding: 8bit 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 08:18:35 -0000 Archived-At: List-Archive: List-Post: 10.12.2019 3:07, Dmitry V. Levin пишет: > On Mon, Dec 09, 2019 at 10:08:42AM +0300, Aleksei Nikiforov wrote: >> 09.12.2019 2:21, Dmitry V. Levin пишет: >>> 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; >>>> >>>> - 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. > > Sorry, I don't find this argument convincing. > I have experienced no const issues in my version of this fix. > Your version is using C-style casts in C++ code. Of course, I could use C-style casts or const_cast-s too to work around const correctness issues (i.e. to just hide these issues), and it'd work like your version. But I'd like to remind you that APT is C++ project, not a C project. What might be ok for C is sometimes a dirty ugly hack in modern C++. >>> [...] >>>> @@ -301,7 +302,7 @@ std::experimental::optional DynamicMMap::Allocate(unsigned long Item >>>> Pool* oldPools = Pools; >>>> auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize); >>>> if (Pools != oldPools) >>>> - I += Pools - oldPools; >>>> + I = RebasePointer(I, oldPools, Pools); >>>> >>>> // 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. > > We normally try to write code that raises less questions. > In that case it's better to keep order from my patch, isn't it? Practically it's fine either way, but theoretically that order is superior. >>> By the way, in this and other similar cases, >>> is there any reason for "Pools != oldPools" check? >>> Is RebasePointer incapable of handling this, or is it an optimization? >>> >> >> 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.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_base) >>>> +{ >>>> + 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_base) + (reinterpret_cast(ptr) - reinterpret_cast(old_base))); >>>> +} >>>> + >>>> +#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. > > 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 = 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. > And it's opposite for me. I prefer to explicitly see when variable is changed. And for all calls it looks exactly same: no matter how it's used, new pointer is returned from function as a result of function. Interface uniformity, obviousness and predictability is important. > > > _______________________________________________ > Devel mailing list > Devel@lists.altlinux.org > https://lists.altlinux.org/mailman/listinfo/devel >