[devel] [SCM] packages/apt: heads/rework-dynamic-mmap

[Ivan Zakharyaschev]

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<unsigned long>()
> > > +                         : 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<pkgCache::PrvIterator> 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. Это же понятнее и 
> лишних вопросов не вызвает: что потом с этой частично осмысленно 
> заполненной, частично незаполненной структурой произойдёт и кто её в таком 
> виде увидит дальше.