From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 29 Jan 2020 19:32:35 +0300 (MSK) From: Ivan Zakharyaschev To: ALT Linux Team development discussions In-Reply-To: Message-ID: References: <20200129012150.83E7E8440710@gitery.altlinux.org> <45228f59-3529-a3ee-7eb7-67eac012ffda@altlinux.org> User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="1807885841-1836049935-1580315555=:6363" Cc: Anton Farygin , Aleksei Nikiforov , "Vladimir D. Seleznev" Subject: Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap 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: Wed, 29 Jan 2020 16:32:36 -0000 Archived-At: List-Archive: List-Post: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1807885841-1836049935-1580315555=:6363 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT On Wed, 29 Jan 2020, Ivan Zakharyaschev wrote: > On Wed, 29 Jan 2020, Aleksei Nikiforov wrote: > > Вот второй фрагмент изменений под вопросом: > > > > > @@ -544,18 +562,24 @@ bool > > pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, > > > #endif > > > > > > // Get a structure > > > - unsigned long Provides = > > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > > - if (Provides == 0) > > > + const auto Provides = > > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > > > + const auto idxVersion = Version.empty() > > > + ? std::experimental::optional() > > > + : WriteString(Version); > > > + if (!Provides || (!Version.empty() && !idxVersion)) > > > return false; > > > Cache.HeaderP->ProvidesCount++; > > > > > > // Fill it in > > > - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + > > Provides,Cache.PkgP); > > > + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + > > *Provides,Cache.PkgP); > > > Prv->Version = Ver.Index(); > > > Prv->NextPkgProv = Ver->ProvidesList; > > > Ver->ProvidesList = Prv.Index(); > > > - if (Version.empty() == false && (Prv->ProvideVersion = > > WriteString(Version)) == 0) > > > - return false; > > > + > > > + if (Version.empty() == false) > > > + { > > > + Prv->ProvideVersion = *idxVersion; > > > + } > > > > > > // Locate the target package > > > pkgCache::PkgIterator Pkg; У меня возник вопрос: а в случае, если Version.empty() истинно, нужно ли как-то заполнять Prv->ProvideVersion специально? Раньше (то, что минусами отмечено) в таком случае Prv->ProvideVersion никак не трогали (потому что происходил shortcut в вычислении &&). > > Здесь мне следующее условие кажется неоправданно усложнённым и трудночитаемым > > по сравнение с кодом из моего изменения: > > > > if (!Provides || (!Version.empty() && !idxVersion)) > > > > Конечно, за счёт этого в условии "if (Version.empty() == false)" код сокращён > > заметно по сравнению с моей версией, но все те строки, хоть их было и больше, > > были простыми. Замена их на несколько усложнённых строк - не то, что я выбрал > > бы в данном случае. Или в данном случае я тоже пропустил какую-то причину > > данного изменения? > > Не то, чтобы есть практические причины, но вызывало удивление и вопрос в > твоей версии, что заполняется бессмысленное значение 0 в случае, если > довести эту "транзакцию" по выделению и заполнению памяти не удалось > довести успешно до конца: > > // Get a structure > const auto Provides = > Owner->AllocateInMap(sizeof(pkgCache::Provides)); > - if (Provides == 0) > + if (!Provides) > return false; > Cache.HeaderP->ProvidesCount++; > > // Fill it in > - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + Provides,Cache.PkgP); > + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + *Provides,Cache.PkgP); > Dynamic DynPrv(Prv); > Prv->Version = Ver.Index(); > Prv->NextPkgProv = Ver->ProvidesList; > Ver->ProvidesList = Prv.Index(); > - if (Version.empty() == false && (Prv->ProvideVersion = WriteString(Version)) == 0) > - return false; > + > + if (Version.empty() == false) > + { > + const auto idxVersion = WriteString(Version); > + if (!idxVersion) > + { > + Prv->ProvideVersion = 0; > + return false; > + } > + > + Prv->ProvideVersion = *idxVersion; > + } > > > Чтобы не было удивления от бессмысленного значения, я переписал так, как > ты увидел. Не люблю операции, которые какой-то мусор присваивают, просто > чтобы место чем-то заполнить. > > В моём варианте Prv даже не конструируется и не начинает заполняться, если > не удалось выделить память в Map под строку Version. Это же понятнее и > лишних вопросов не вызвает: что потом с этой частично осмысленно > заполненной, частично незаполненной структурой произойдёт и кто её в таком > виде увидит дальше. --1807885841-1836049935-1580315555=:6363--