Hello! On Thu, 13 Feb 2020, Ivan Zakharyaschev wrote: > > > есть такой hunk касательно DynamicMMap::RawAllocate(): > > > > > > diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc > > > index d7a5c3a68..36dc11524 100644 > > > --- a/apt/apt-pkg/contrib/mmap.cc > > > +++ b/apt/apt-pkg/contrib/mmap.cc > > > @@ -226,6 +245,12 @@ unsigned long DynamicMMap::RawAllocate(unsigned long > > > Size,unsigned long Aln) > > > size in the file. */ > > > unsigned long DynamicMMap::Allocate(unsigned long ItemSize) > > > { > > > + if (ItemSize == 0) > > > + { > > > + _error->Error("Can't allocate an item of size zero"); > > > + return 0; > > > + } > > > + > > > // Look for a matching pool entry > > > Pool *I; > > > Pool *Empty = 0; > > > Во-вторых, я подумал, что это такой класс ошибок, который не возникает > > > в run-time из-за плохих данных или отказа ОС (например, выделить > > > память или прочитать файл) -- т.е. не что-то заранее непредсказуемое > > > для программиста и поэтому требующее специальной обработки в run-time. > > > > > > Если програмист написал всё так, как он хотел, то он не передаст > > > неправильный аргумент (0) в функцию. (Да и понятно, что в реальном > > > коде APT это не ожидается: там обычно этот аргумент заполняется > > > константой sizeof(...).) > > > > > > > Ты упускаешь большой момент. mmap.h - публичный заголовок библиотеки libapt. > > Помимо самого apt, им пользуется ещё и немало клиентов. Даже если в самом apt > > не найдётся способа неправильно вызвать данную функцию, это ничего не значит. > > Не проверять входные данные - плохая идея. > > Всё равно это ответственность программиста: правильно вызвать. > > Я выше написал, в каких случаях я счиатю нужным проверять данные. > > > > Я бы предложил такие требования (имеющие характер спецификации того, > > > что программист задумал реализовать) оформлять просто с помощью > > > assert. > > > > > > > assert в не-debug сборке отсутствует. Потому чуть более чем полностью > > бесполезен не во время разработки приложения. > > Я так и задумывал. > > > > Это не средство проверки входных > > данных. Поэтому я против. > > > > > Так будет легче понимать код: сразу ясно, что выполнение этого условия > > > (важного для корректности кода в теле функции) просто гарантируется > > > тем, как весь наш код написан и вообще-то может быть обосновано ещё до > > > run-time (просто глядя на исходный код, на том же этапе работы, что и > > > запускаем компилятор). Появилась идея, что есть способ здесь не просто ответственность возложить на программиста на словах (вызывать Allocate() с ненулевым аргументом), но и формально предложить такой способ программирования, где не будет возможности делать не так, как задумано. Да и попроще вызовы получаются. Ветка alloc-templates в http://git.altlinux.org/people/imz/packages/apt.git (Развивая это, можно RawAllocate() превратить в какой-нибудь template RawAllocateArray(unsigned long ItemCount), там, правда, Pools нетривиально надо будет переделать. Пришлось, правда, перенести определение из .cc в .h и патчи дальнейшие неудобно прикладывать; но можно их ifdef-ами обложить в .cc и заинклудить в .h.) >From 89499e5810575336e6a3b02d71d6c323321a0eca Mon Sep 17 00:00:00 2001 From: Ivan Zakharyaschev Date: Fri, 14 Feb 2020 04:04:42 +0300 Subject: [PATCH] don't pass compile-time constants as an argument to DynamicMMap::Allocate() The argument was used for the size of the item we want to allocate. Instead of an argument, use a parameter for the template DynamicMMap::Allocate() which denotes the type of the item. Now, the truth of our assertion that the size is not zero is evident (and checked by the compiler). Note that this change may also potentially lead to more optimized code due to compile-time specialization for each particular size. --- apt/apt-pkg/contrib/mmap.h | 12 ++++++------ apt/apt-pkg/pkgcachegen.cc | 19 ++++++++++--------- apt/apt-pkg/pkgcachegen.h | 3 ++- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h index 5b14ac9d9..7ec0fb8b1 100644 --- a/apt/apt-pkg/contrib/mmap.h +++ b/apt/apt-pkg/contrib/mmap.h @@ -94,7 +94,8 @@ class DynamicMMap : public MMap // Allocation std::experimental::optional RawAllocate(unsigned long Size,unsigned long Aln = 0); - std::experimental::optional Allocate(unsigned long ItemSize); + template + std::experimental::optional Allocate(); std::experimental::optional WriteString(const char *String,unsigned long Len = std::numeric_limits::max()); inline std::experimental::optional WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; void UsePools(Pool &P,unsigned int Count) {Pools = &P; PoolCount = Count;}; @@ -107,17 +108,16 @@ class DynamicMMap : public MMap /* Templates */ #include -#include // DynamicMMap::Allocate - Pooled aligned allocation /*{{{*/ // --------------------------------------------------------------------- /* This allocates an Item of size ItemSize so that it is aligned to its size in the file. */ -std::experimental::optional DynamicMMap::Allocate(unsigned long ItemSize) +template +std::experimental::optional DynamicMMap::Allocate() { - assert(ItemSize != 0); /* Actually, we are always called with sizeof(...) - compile-time non-zero constant as the argument. - */ + constexpr unsigned long ItemSize = sizeof(T); + static_assert(ItemSize != 0); // Look for a matching pool entry Pool *I; diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 4bbab07bf..2a75f31e6 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -112,8 +112,9 @@ std::experimental::optional pkgCacheGenerator::WriteStringInMap(c return Map.WriteString(String); } /*}}}*/ -std::experimental::optional pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/ - return Map.Allocate(size); +template +std::experimental::optional pkgCacheGenerator::AllocateInMap() {/*{{{*/ + return Map.Allocate(); } /*}}}*/ // CacheGenerator::MergeList - Merge the package list /*{{{*/ @@ -376,7 +377,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, const string &Nam #endif // Get a structure - const auto Package = AllocateInMap(sizeof(pkgCache::Package)); + const auto Package = AllocateInMap(); const auto idxName = WriteStringInMap(Name); if ((!Package) || (!idxName)) return false; @@ -429,7 +430,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return true; // Get a structure - const auto VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); + const auto VerFile = AllocateInMap(); if (!VerFile) return false; @@ -460,7 +461,7 @@ std::experimental::optional pkgCacheGenerator::NewVersion(pkgCach unsigned long Next) { // Get a structure - const auto Version = AllocateInMap(sizeof(pkgCache::Version)); + const auto Version = AllocateInMap(); const auto idxVerStr = WriteStringInMap(VerStr); if ((!Version) || (!idxVerStr)) return std::experimental::nullopt; @@ -487,7 +488,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, pkgCache &Cache = Owner->Cache; // Get a structure - const auto Dependency = Owner->AllocateInMap(sizeof(pkgCache::Dependency)); + const auto Dependency = Owner->AllocateInMap(); if (!Dependency) return false; @@ -574,7 +575,7 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, } } // Get a structure - const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); + const auto Provides = Owner->AllocateInMap(); if (!Provides) return false; /* Note: @@ -618,7 +619,7 @@ bool pkgCacheGenerator::SelectFile(const string &File, const string &Site, unsigned long Flags) { // Get some space for the structure - const auto idxFile = AllocateInMap(sizeof(*CurrentFile)); + const auto idxFile = AllocateInMap(); const auto idxFileName = WriteStringInMap(File); const auto idxSite = WriteUniqString(Site); const auto idxIndexType = WriteUniqString(Index.GetType()->Label); @@ -676,7 +677,7 @@ std::experimental::optional pkgCacheGenerator::WriteUniqString(co } // Get a structure - const auto Item = AllocateInMap(sizeof(pkgCache::StringItem)); + const auto Item = AllocateInMap(); const auto idxString = WriteStringInMap(S, Size); if ((!Item) || (!idxString)) return std::experimental::nullopt; diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h index b789177ef..73252802b 100644 --- a/apt/apt-pkg/pkgcachegen.h +++ b/apt/apt-pkg/pkgcachegen.h @@ -39,7 +39,8 @@ class pkgCacheGenerator std::experimental::optional WriteStringInMap(const std::string &String) { return WriteStringInMap(String.c_str(), String.length()); }; std::experimental::optional WriteStringInMap(const char *String); std::experimental::optional WriteStringInMap(const char *String, unsigned long Len); - std::experimental::optional AllocateInMap(unsigned long size); + template + std::experimental::optional AllocateInMap(); public: -- 2.21.0