From 48aab7ab86cc042504a17bd304267a1feb39aa7f Mon Sep 17 00:00:00 2001 From: Pedro Maciel Date: Thu, 19 Sep 2024 23:46:24 +0100 Subject: [PATCH 1/5] eckit::CacheManager modernise --- src/eckit/container/CacheManager.cc | 95 ++++++++++++++--------------- src/eckit/container/CacheManager.h | 75 +++++++++++------------ 2 files changed, 82 insertions(+), 88 deletions(-) diff --git a/src/eckit/container/CacheManager.cc b/src/eckit/container/CacheManager.cc index 5d3bdd47b..e8c8943f9 100644 --- a/src/eckit/container/CacheManager.cc +++ b/src/eckit/container/CacheManager.cc @@ -8,6 +8,8 @@ * does it submit to any jurisdiction. */ +#include "eckit/container/CacheManager.h" + #include #include @@ -16,9 +18,7 @@ #include #include "eckit/container/BTree.h" -#include "eckit/container/CacheManager.h" #include "eckit/log/Bytes.h" -#include "eckit/os/AutoUmask.h" #include "eckit/runtime/Main.h" namespace eckit { @@ -28,8 +28,7 @@ namespace eckit { CacheManagerBase::CacheManagerBase(const std::string& loaderName, size_t maxCacheSize, const std::string& extension) : loaderName_(loaderName), maxCacheSize_(maxCacheSize), extension_(extension) {} -CacheManagerBase::~CacheManagerBase() {} - +CacheManagerBase::~CacheManagerBase() = default; std::string CacheManagerBase::loader() const { return loaderName_; @@ -45,29 +44,27 @@ static bool compare(const T& a, const T& b) { return a.second.last_ < b.second.last_; } -static bool sub_path_of(const eckit::PathName& base, const eckit::PathName& path) { - +static bool sub_path_of(const PathName& base, const PathName& path) { std::string subp = base.asString(); std::string p = path.asString(); - // eckit::Log::debug() << "subp " << subp << std::endl; - // eckit::Log::debug() << "p " << p << std::endl; + // Log::debug() << "subp " << subp << std::endl; + // Log::debug() << "p " << p << std::endl; std::size_t f = p.find(subp); return f == 0; } -void CacheManagerBase::rescanCache(const eckit::PathName& base) const { - +void CacheManagerBase::rescanCache(const PathName& base) const { ASSERT(btree_); - eckit::PathName db(base / "cache-manager.btree"); - eckit::PathName mapping(base / "cache-manager.mapping"); // already under lock from callee + PathName db(base / "cache-manager.btree"); + PathName mapping(base / "cache-manager.mapping"); // already under lock from callee cache_btree_t& btree = *btree_; - eckit::Log::info() << "CACHE-MANAGER cleanup " << db << ", rebuilding index" << std::endl; + Log::info() << "CACHE-MANAGER cleanup " << db << ", rebuilding index" << std::endl; std::vector files; @@ -77,16 +74,16 @@ void CacheManagerBase::rescanCache(const eckit::PathName& base) const { std::ofstream out(mapping.asString().c_str(), std::ios::app); - for (std::vector::const_iterator j = files.begin(); j != files.end(); ++j) { - if (j->extension() != extension_) { + for (const auto & file : files) { + if (file.extension() != extension_) { continue; } - eckit::Log::info() << "CACHE-MANAGER cleanup " << db << ", indexing " << (*j) << std::endl; + Log::info() << "CACHE-MANAGER cleanup " << db << ", indexing " << file << std::endl; - MD5 md5(*j); + MD5 md5(file); - out << md5.digest() << " " << (*j) << std::endl; + out << md5.digest() << " " << file << std::endl; cache_key_t key(md5.digest()); cache_entry_t entry = {0, 0, 0}; @@ -95,9 +92,9 @@ void CacheManagerBase::rescanCache(const eckit::PathName& base) const { cache_entry_t total_entry = {0, 0, 0}; if (!btree.get(key, entry)) { - entry.size_ = size_t((*j).size()); + entry.size_ = size_t(file.size()); entry.count_ = 1; - entry.last_ = ::time(0); + entry.last_ = ::time(nullptr); btree.set(key, entry); if (btree.get(total_key, total_entry)) { @@ -109,7 +106,7 @@ void CacheManagerBase::rescanCache(const eckit::PathName& base) const { } } -void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& path) const { +void CacheManagerBase::touch(const PathName& base, const PathName& path) const { // 1- Do we do it (bool) // 2- where do we store the data (path) @@ -127,8 +124,8 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& AutoUmask umask(0); - eckit::PathName db(base / "cache-manager.btree"); - eckit::PathName mapping(base / "cache-manager.mapping"); + PathName db(base / "cache-manager.btree"); + PathName mapping(base / "cache-manager.mapping"); try { @@ -146,8 +143,8 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& if (!btree.get(key, entry)) { // entry not in cache-manager.btree - eckit::FileLock lock(mapping); - eckit::AutoLock locker(lock); + FileLock lock(mapping); + AutoLock locker(lock); if (mapping.size() == Length(0)) { rescanCache(base); @@ -174,12 +171,12 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& size_t remove = total_entry.size_ - maxCacheSize_; // Cleanup - eckit::Log::info() << "CACHE-MANAGER cleanup " << db << ", size is " << eckit::Bytes(total_entry.size_) - << ", max size is " << eckit::Bytes(maxCacheSize_) << ", removing " - << eckit::Bytes(remove) << std::endl; + Log::info() << "CACHE-MANAGER cleanup " << db << ", size is " << Bytes(total_entry.size_) + << ", max size is " << Bytes(maxCacheSize_) << ", removing " + << Bytes(remove) << std::endl; - std::map md5_to_path; + std::map md5_to_path; std::ifstream in(mapping.asString().c_str()); std::string s, t; @@ -187,10 +184,10 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& while (in >> s >> t) { - eckit::PathName p(t); + PathName p(t); if (s.length() != MD5_DIGEST_LENGTH * 2 || not sub_path_of(base, p)) { - eckit::Log::warning() + Log::warning() << "CACHE-MANAGER cleanup " << mapping << ", invalid entry [" << s << "] and [" << t << "], ignoring but will rebuild index later" << std::endl; rescan = true; @@ -222,13 +219,13 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& continue; } - std::map::iterator j = md5_to_path.find(p.first); + std::map::iterator j = md5_to_path.find(p.first); if (j == md5_to_path.end()) { // in the btree, but not file mapping. Path may exist on disk rescan = true; continue; } - const eckit::PathName& file = (*j).second; + const PathName& file = (*j).second; size_t unlinked = 0; @@ -249,17 +246,17 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& deleted += unlinked; - eckit::Log::warning() - << "CACHE-MANAGER cleanup " << file << ", deleted: " << eckit::Bytes(unlinked) << std::endl; + Log::warning() + << "CACHE-MANAGER cleanup " << file << ", deleted: " << Bytes(unlinked) << std::endl; } if (deleted < remove) { - eckit::Log::warning() << "CACHE-MANAGER cleanup " << mapping << ", could not delete enough space" - << " total is " << eckit::Bytes(total_entry.size_) << std::endl; + Log::warning() << "CACHE-MANAGER cleanup " << mapping << ", could not delete enough space" + << " total is " << Bytes(total_entry.size_) << std::endl; } if (rescan) { - eckit::Log::warning() << "CACHE-MANAGER cleanup " << mapping + Log::warning() << "CACHE-MANAGER cleanup " << mapping << ", inconsitent cache index, needs rebuilding" << std::endl; // TODO: rebuild index + mapping from directory scan @@ -270,7 +267,7 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& } } - entry.last_ = ::time(0); + entry.last_ = ::time(nullptr); entry.count_++; btree.set(key, entry); @@ -278,7 +275,7 @@ void CacheManagerBase::touch(const eckit::PathName& base, const eckit::PathName& Log::debug() << "CACHE-MANAGER touched path " << path << std::endl; } catch (std::exception& e) { - eckit::Log::error() << "Error updating " << db << ", turning off" << eckit::Log::syserr << std::endl; + Log::error() << "Error updating " << db << ", turning off" << Log::syserr << std::endl; const_cast(this)->maxCacheSize_ = 0; } } @@ -289,10 +286,10 @@ bool CacheManagerBase::writable(const PathName& path) const { //---------------------------------------------------------------------------------------------------------------------- -static eckit::PathName lockFile(const std::string& path) { - eckit::AutoUmask umask(0); +static PathName lockFile(const std::string& path) { + AutoUmask umask(0); - eckit::PathName lock(path + ".lock"); + PathName lock(path + ".lock"); lock.touch(); return lock; } @@ -301,23 +298,23 @@ CacheManagerFileSemaphoreLock::CacheManagerFileSemaphoreLock(const std::string& path_(lockFile(path)), lock_(path_) {} void CacheManagerFileSemaphoreLock::lock() { - eckit::AutoUmask umask(0); + AutoUmask umask(0); - eckit::Log::info() << "Wait for lock " << path_ << std::endl; + Log::info() << "Wait for lock " << path_ << std::endl; lock_.lock(); - eckit::Log::info() << "Got lock " << path_ << std::endl; + Log::info() << "Got lock " << path_ << std::endl; - std::string hostname = eckit::Main::hostname(); + std::string hostname = Main::hostname(); std::ofstream os(path_.asString().c_str()); os << hostname << " " << ::getpid() << std::endl; } void CacheManagerFileSemaphoreLock::unlock() { - eckit::AutoUmask umask(0); + AutoUmask umask(0); - eckit::Log::info() << "Unlock " << path_ << std::endl; + Log::info() << "Unlock " << path_ << std::endl; std::ofstream os(path_.asString().c_str()); os << std::endl; lock_.unlock(); diff --git a/src/eckit/container/CacheManager.h b/src/eckit/container/CacheManager.h index adcc94743..0bdcea680 100644 --- a/src/eckit/container/CacheManager.h +++ b/src/eckit/container/CacheManager.h @@ -17,23 +17,20 @@ #include -#include #include #include #include "eckit/config/LibEcKit.h" -#include "eckit/config/Resource.h" -#include "eckit/eckit.h" +#include "eckit/exception/Exceptions.h" #include "eckit/filesystem/PathExpander.h" #include "eckit/filesystem/PathName.h" #include "eckit/io/FileLock.h" -#include "eckit/memory/NonCopyable.h" +#include "eckit/log/Log.h" #include "eckit/os/AutoUmask.h" #include "eckit/os/Semaphore.h" #include "eckit/thread/AutoLock.h" #include "eckit/types/FixedString.h" #include "eckit/utils/MD5.h" -#include "eckit/utils/StringTools.h" #include "eckit/utils/Tokenizer.h" namespace eckit { @@ -47,10 +44,16 @@ class BTreeLock; /// Filesystem Cache Manager -class CacheManagerBase : private NonCopyable { - -public: // methods +class CacheManagerBase { +public: CacheManagerBase(const std::string& loaderName, size_t maxCacheSize, const std::string& extension); + + CacheManagerBase(const CacheManagerBase&) = delete; + CacheManagerBase(CacheManagerBase&&) = delete; + + CacheManagerBase& operator=(const CacheManagerBase&) = delete; + CacheManagerBase& operator=(CacheManagerBase&&) = delete; + ~CacheManagerBase(); std::string loader() const; @@ -61,12 +64,12 @@ class CacheManagerBase : private NonCopyable { bool writable(const PathName& path) const; -private: // members +private: std::string loaderName_; size_t maxCacheSize_; std::string extension_; - typedef FixedString cache_key_t; + using cache_key_t = FixedString; struct cache_entry_t { size_t size_; @@ -74,7 +77,7 @@ class CacheManagerBase : private NonCopyable { time_t last_; }; - typedef BTree cache_btree_t; + using cache_btree_t = BTree; mutable std::unique_ptr btree_; }; @@ -85,7 +88,7 @@ class CacheManagerBase : private NonCopyable { class CacheManagerNoLock { public: - CacheManagerNoLock() {} + CacheManagerNoLock() = default; void lock() {} void unlock() {} }; @@ -93,12 +96,11 @@ class CacheManagerNoLock { //---------------------------------------------------------------------------------------------------------------------- class CacheManagerFileSemaphoreLock { - PathName path_; eckit::Semaphore lock_; public: - CacheManagerFileSemaphoreLock(const std::string& path); + explicit CacheManagerFileSemaphoreLock(const std::string& path); void lock(); void unlock(); }; @@ -106,11 +108,10 @@ class CacheManagerFileSemaphoreLock { //---------------------------------------------------------------------------------------------------------------------- class CacheManagerFileFlock { - eckit::FileLock lock_; public: - CacheManagerFileFlock(const std::string& path); + explicit CacheManagerFileFlock(const std::string& path); void lock(); void unlock(); }; @@ -122,7 +123,7 @@ template class CacheManager : public CacheManagerBase { public: // methods - typedef typename Traits::value_type value_type; + using value_type = typename Traits::value_type; class CacheContentCreator { public: @@ -130,7 +131,7 @@ class CacheManager : public CacheManagerBase { virtual void create(const PathName&, value_type& value, bool& saved) = 0; }; - typedef std::string key_t; + using key_t = std::string; public: // methods explicit CacheManager(const std::string& loaderName, const std::string& roots, bool throwOnCacheMiss, @@ -187,7 +188,7 @@ CacheManager::CacheManager(const std::string& loaderName, const std::str << "CACHE-MANAGER " << Traits::name() << ", " << p.dirName() << " not writable" << std::endl; } } - catch (FailedSystemCall&) { /* ignore */ + catch (FailedSystemCall&) { // ignore } } @@ -201,14 +202,12 @@ CacheManager::CacheManager(const std::string& loaderName, const std::str } template -bool CacheManager::get(const key_t& key, PathName& v) const { - - for (auto& root : roots_) { - PathName p = entry(key, root); +bool CacheManager::get(const key_t& key, PathName& path) const { + for (const auto& root : roots_) { + auto p = entry(key, root); if (p.exists()) { - - v = p; + path = p; Log::debug() << "CACHE-MANAGER found path " << p << std::endl; @@ -223,9 +222,8 @@ bool CacheManager::get(const key_t& key, PathName& v) const { oss << "CacheManager cache miss: key=" << key << ", tried:"; const char* sep = " "; - for (auto& root : roots_) { - PathName p = entry(key, root); - oss << sep << p; + for (const auto& root : roots_) { + oss << sep << entry(key, root); sep = ", "; } @@ -239,20 +237,19 @@ template PathName CacheManager::base(const std::string& root) const { std::ostringstream oss; oss << root << "/" << Traits::name(); - return PathName(oss.str()); + return oss.str(); } template PathName CacheManager::entry(const key_t& key, const std::string& root) const { std::ostringstream oss; oss << base(root).asString() << "/" << Traits::version() << "/" << key << Traits::extension(); - return PathName(oss.str()); + return oss.str(); } template PathName CacheManager::stage(const key_t& key, const PathName& root) const { - - PathName p = entry(key, root); + auto p = entry(key, root); AutoUmask umask(0); // FIXME: mask does not seem to affect first level directory p.dirName().mkdir(0777); // ensure directory exists @@ -262,11 +259,13 @@ PathName CacheManager::stage(const key_t& key, const PathName& root) con } template -bool CacheManager::commit(const key_t& key, const PathName& tmpfile, const PathName& root) const { - PathName file = entry(key, root); +bool CacheManager::commit(const key_t& key, const PathName& path, const PathName& root) const { + // path is the temporary file + + auto file = entry(key, root); try { - SYSCALL(::chmod(tmpfile.asString().c_str(), 0444)); - PathName::rename(tmpfile, file); + SYSCALL(::chmod(path.asString().c_str(), 0444)); + PathName::rename(path, file); } catch (FailedSystemCall& e) { // ignore failed system call -- another process nay have created the file meanwhile Log::debug() << "Failed rename of cache file -- " << e.what() << std::endl; @@ -277,7 +276,6 @@ bool CacheManager::commit(const key_t& key, const PathName& tmpfile, con template PathName CacheManager::getOrCreate(const key_t& key, CacheContentCreator& creator, value_type& value) const { - PathName path; if (get(key, path)) { @@ -320,8 +318,7 @@ PathName CacheManager::getOrCreate(const key_t& key, CacheContentCreator Traits::load(*this, value, path); } else { - Log::debug() << "Loading cache file " << file << " (created by another process)" - << std::endl; + Log::debug() << "Loading cache file " << file << " (created by another process)" << std::endl; // touch() is done in the (successful) get() above Traits::load(*this, value, path); From f2e6189e76c5b956927f1c8cc838eb383974372b Mon Sep 17 00:00:00 2001 From: Pedro Maciel Date: Wed, 29 Jan 2025 09:30:21 +0000 Subject: [PATCH 2/5] eckit::geo --- src/eckit/geo/Grid.h | 5 ++++- src/eckit/geo/spec/Custom.h | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/eckit/geo/Grid.h b/src/eckit/geo/Grid.h index e97dd80fa..63bb6f428 100644 --- a/src/eckit/geo/Grid.h +++ b/src/eckit/geo/Grid.h @@ -134,7 +134,10 @@ class Grid { virtual iterator cend() const = 0; NextIterator next_iterator() const { return {cbegin().release(), cend().release()}; } - NextIterator* make_next_iterator() const { return new NextIterator{cbegin().release(), cend().release()}; } + + [[nodiscard]] NextIterator* make_next_iterator() const { + return new NextIterator{cbegin().release(), cend().release()}; + } const Spec& spec() const; std::string spec_str() const { return spec().str(); } diff --git a/src/eckit/geo/spec/Custom.h b/src/eckit/geo/spec/Custom.h index b2aca90a7..e2f9fe674 100644 --- a/src/eckit/geo/spec/Custom.h +++ b/src/eckit/geo/spec/Custom.h @@ -113,7 +113,7 @@ class Custom final : public Spec { // -- Class methods - static Custom* make_from_value(const Value&); + [[nodiscard]] static Custom* make_from_value(const Value&); private: // -- Members From bc1f531aae4a7209b4bf85ee6606c9b8e1d07dcf Mon Sep 17 00:00:00 2001 From: Pedro Maciel Date: Wed, 29 Jan 2025 14:30:23 +0000 Subject: [PATCH 3/5] eckit::geo::Spec --- src/eckit/geo/Spec.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/eckit/geo/Spec.h b/src/eckit/geo/Spec.h index bd913d06a..1a4d0a1ed 100644 --- a/src/eckit/geo/Spec.h +++ b/src/eckit/geo/Spec.h @@ -35,6 +35,9 @@ class Spec : public Parametrisation { Spec& operator=(const Spec&) = delete; Spec& operator=(Spec&&) = delete; + bool operator==(const Spec& other) const { return str() == other.str(); } + bool operator!=(const Spec& other) const { return !operator==(other); } + std::string get_string(const std::string& name) const; bool get_bool(const std::string& name) const; int get_int(const std::string& name) const; From fcd9498b2467adc59151e41014625db80383020a Mon Sep 17 00:00:00 2001 From: Pedro Maciel Date: Thu, 30 Jan 2025 00:22:32 +0000 Subject: [PATCH 4/5] eckit::geo rename SpecNotFound to SpecError --- src/eckit/geo/Exceptions.cc | 4 ++-- src/eckit/geo/Exceptions.h | 4 ++-- src/eckit/geo/Figure.cc | 2 +- src/eckit/geo/Grid.cc | 2 +- src/eckit/geo/Increments.cc | 2 +- src/eckit/geo/Shape.cc | 2 +- src/eckit/geo/Spec.cc | 2 +- src/eckit/geo/spec/Custom.cc | 2 +- tests/geo/spec.cc | 2 +- tests/geo/spec_custom.cc | 10 +++++----- 10 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/eckit/geo/Exceptions.cc b/src/eckit/geo/Exceptions.cc index bd6aaf27c..4796d1d7c 100644 --- a/src/eckit/geo/Exceptions.cc +++ b/src/eckit/geo/Exceptions.cc @@ -21,8 +21,8 @@ ProjectionError::ProjectionError(const std::string& what, const CodeLocation& lo }; -SpecNotFound::SpecNotFound(const std::string& what, const CodeLocation& loc) : Exception(loc) { - reason("SpecNotFound: [" + what + "], in " + loc.asString()); +SpecError::SpecError(const std::string& what, const CodeLocation& loc) : Exception(loc) { + reason("SpecError: [" + what + "], in " + loc.asString()); }; diff --git a/src/eckit/geo/Exceptions.h b/src/eckit/geo/Exceptions.h index c4f0f5f0d..18eced695 100644 --- a/src/eckit/geo/Exceptions.h +++ b/src/eckit/geo/Exceptions.h @@ -36,9 +36,9 @@ class ProjectionError : public geo::Exception { }; -class SpecNotFound : public geo::Exception { +class SpecError : public geo::Exception { public: - explicit SpecNotFound(const std::string&, const CodeLocation&); + explicit SpecError(const std::string&, const CodeLocation&); }; diff --git a/src/eckit/geo/Figure.cc b/src/eckit/geo/Figure.cc index 374fc5293..b64546487 100644 --- a/src/eckit/geo/Figure.cc +++ b/src/eckit/geo/Figure.cc @@ -128,7 +128,7 @@ Figure* FigureFactory::make_from_spec_(const Spec& spec) const { } Log::error() << "Figure: cannot build figure without 'R' or 'a', 'b'" << std::endl; - throw exception::SpecNotFound("Figure: cannot build figure without 'R' or 'a', 'b'", Here()); + throw exception::SpecError("Figure: cannot build figure without 'R' or 'a', 'b'", Here()); } diff --git a/src/eckit/geo/Grid.cc b/src/eckit/geo/Grid.cc index 1432609ea..134fae626 100644 --- a/src/eckit/geo/Grid.cc +++ b/src/eckit/geo/Grid.cc @@ -234,7 +234,7 @@ const Grid* GridFactory::make_from_spec_(const Spec& spec) const { } list(Log::error() << "Grid: cannot build grid without 'type', choices are: "); - throw exception::SpecNotFound("Grid: cannot build grid without 'type'", Here()); + throw exception::SpecError("Grid: cannot build grid without 'type'", Here()); } diff --git a/src/eckit/geo/Increments.cc b/src/eckit/geo/Increments.cc index 7964bb5e9..06e6476ca 100644 --- a/src/eckit/geo/Increments.cc +++ b/src/eckit/geo/Increments.cc @@ -31,7 +31,7 @@ Increments Increments::make_from_spec(const Spec& spec) { return {dx, dy}; } - throw exception::SpecNotFound( + throw exception::SpecError( "'increments' = 'grid' = ['dx', 'dy'] = ['west_east_increment', 'south_north_increment'] = ['dlon', 'dlat'] = " "['dx', 'dy'] expected", Here()); diff --git a/src/eckit/geo/Shape.cc b/src/eckit/geo/Shape.cc index a4dd127b4..d6cef0af9 100644 --- a/src/eckit/geo/Shape.cc +++ b/src/eckit/geo/Shape.cc @@ -29,7 +29,7 @@ Shape Shape::make_from_spec(const Spec& spec) { return {nx, ny}; } - throw exception::SpecNotFound("'shape' = ['nlon', 'nlat'] = ['nx', 'ny'] expected", Here()); + throw exception::SpecError("'shape' = ['nlon', 'nlat'] = ['nx', 'ny'] expected", Here()); } diff --git a/src/eckit/geo/Spec.cc b/src/eckit/geo/Spec.cc index 734d1fca6..0a81c6a33 100644 --- a/src/eckit/geo/Spec.cc +++ b/src/eckit/geo/Spec.cc @@ -31,7 +31,7 @@ static inline T _get_d(const Spec& spec, const std::string& name, const T& _defa template static inline T _get_t(const Spec& spec, const std::string& name) { T value{}; - return spec.get(name, value) ? value : throw exception::SpecNotFound(name, Here()); + return spec.get(name, value) ? value : throw exception::SpecError(name, Here()); } diff --git a/src/eckit/geo/spec/Custom.cc b/src/eckit/geo/spec/Custom.cc index 18a150f33..e97e0dd6a 100644 --- a/src/eckit/geo/spec/Custom.cc +++ b/src/eckit/geo/spec/Custom.cc @@ -348,7 +348,7 @@ const Custom::custom_ptr& Custom::custom(const std::string& name) const { } } - throw exception::SpecNotFound("Custom::get(" + name + ") -> custom_type& ", Here()); + throw exception::SpecError("Custom::get(" + name + ") -> custom_type& ", Here()); } diff --git a/tests/geo/spec.cc b/tests/geo/spec.cc index 26da7bcd1..1d0f1bb5e 100644 --- a/tests/geo/spec.cc +++ b/tests/geo/spec.cc @@ -92,7 +92,7 @@ CASE("user -> type") { std::unique_ptr grid(GridFactory::build(*spec)); EXPECT(grid); } - catch (const exception::SpecNotFound& e) { + catch (const exception::SpecError& e) { EXPECT(refspec.empty() /*BAD*/); } catch (const BadParameter& e) { diff --git a/tests/geo/spec_custom.cc b/tests/geo/spec_custom.cc index 4fc744723..997c76fdb 100644 --- a/tests/geo/spec_custom.cc +++ b/tests/geo/spec_custom.cc @@ -136,11 +136,11 @@ age: 33 EXPECT(nested.has_custom("a")); EXPECT_NOT(nested.has_custom("a?")); - EXPECT_THROWS_AS(nested.custom("a?"), exception::SpecNotFound); + EXPECT_THROWS_AS(nested.custom("a?"), exception::SpecError); EXPECT(nested.custom("a")->has_custom("b")); EXPECT_NOT(nested.custom("a")->has_custom("b?")); - EXPECT_THROWS_AS(nested.custom("a")->custom("b?"), exception::SpecNotFound); + EXPECT_THROWS_AS(nested.custom("a")->custom("b?"), exception::SpecError); const auto& b = nested.custom("a")->custom("b"); ASSERT(b); @@ -281,7 +281,7 @@ CASE("Spec <- Custom") { c.set("foo", two); EXPECT(c.has("foo")); - EXPECT_THROWS_AS(c.get_int("foo"), exception::SpecNotFound); // cannot access as int + EXPECT_THROWS_AS(c.get_int("foo"), exception::SpecError); // cannot access as int EXPECT(::eckit::types::is_approximately_equal(c.get_double("foo"), two)); EXPECT(c.get_string("foo") == std::to_string(two)); @@ -298,8 +298,8 @@ CASE("Spec <- Custom") { EXPECT(d.has("foo")); EXPECT(d.get_string("foo") == three); - EXPECT_THROWS_AS(d.get_int("foo"), exception::SpecNotFound); // cannot access as int - EXPECT_THROWS_AS(d.get_double("foo"), exception::SpecNotFound); // cannot access as real + EXPECT_THROWS_AS(d.get_int("foo"), exception::SpecError); // cannot access as int + EXPECT_THROWS_AS(d.get_double("foo"), exception::SpecError); // cannot access as real d.set("foo", one); EXPECT(d.get_int("foo") == one); From e42fdf83cecd3979c8e3021cffc02deed90f23ba Mon Sep 17 00:00:00 2001 From: Pedro Maciel Date: Thu, 30 Jan 2025 00:23:04 +0000 Subject: [PATCH 5/5] eckit::geo::grid::HEALPix match eccodes tests --- src/eckit/geo/grid/HEALPix.cc | 27 ++++++++++++++++++--------- tests/geo/grid_healpix.cc | 8 +++++++- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/eckit/geo/grid/HEALPix.cc b/src/eckit/geo/grid/HEALPix.cc index 6ceb585b6..1960edc31 100644 --- a/src/eckit/geo/grid/HEALPix.cc +++ b/src/eckit/geo/grid/HEALPix.cc @@ -217,28 +217,37 @@ class Reorder { } // unnamed namespace +static const std::string HEALPIX_ERROR_NSIDE_POSITIVE = "HEALPix: Nside must be greater than zero"; +static const std::string HEALPIX_ERROR_NSIDE_POWER2 = "HEALPix: Nside must be a power of 2"; +static const std::string HEALPIX_ERROR_ORDERING + = "HEALPix: supported ordering: ring, nested (Only orderingConvention are supported)"; + + HEALPix::HEALPix(const Spec& spec) : HEALPix(util::convert_long_to_size_t(spec.get_long("Nside")), [](const std::string& str) { return str == "ring" ? Ordering::healpix_ring : str == "nested" ? Ordering::healpix_nested - : throw AssertionFailed("HEALPix: supported orderings: ring, nested", Here()); + : throw exception::SpecError(HEALPIX_ERROR_ORDERING, Here()); }(spec.get_string("ordering", "ring"))) {} HEALPix::HEALPix(size_t Nside, Ordering ordering) : Reduced(area::BoundingBox{}), Nside_(Nside), ordering_(ordering) { - ASSERT(Nside_ > 0); - ASSERT_MSG(ordering == Ordering::healpix_ring || ordering == Ordering::healpix_nested, - "HEALPix: supported orderings: ring, nested"); + if (Nside_ == 0) { + throw exception::SpecError(HEALPIX_ERROR_NSIDE_POSITIVE, Here()); + } + + if (ordering != Ordering::healpix_ring && ordering != Ordering::healpix_nested) { + throw exception::SpecError(HEALPIX_ERROR_ORDERING, Here()); + } - if (ordering_ == Ordering::healpix_nested) { - ASSERT(is_power_of_2(Nside)); + if (ordering_ == Ordering::healpix_nested && !is_power_of_2(Nside_)) { + throw exception::SpecError(HEALPIX_ERROR_NSIDE_POWER2, Here()); } } Renumber HEALPix::reorder(Ordering ordering) const { - ASSERT_MSG(ordering == Ordering::healpix_ring || ordering == Ordering::healpix_nested, - "HEALPix: supported orderings: ring, nested"); + ASSERT_MSG(ordering == Ordering::healpix_ring || ordering == Ordering::healpix_nested, HEALPIX_ERROR_ORDERING); if (ordering == ordering_) { return Grid::no_reorder(size()); @@ -380,7 +389,7 @@ const std::string& HEALPix::type() const { static const GridRegisterType GRIDTYPE1("HEALPix"); static const GridRegisterType GRIDTYPE2("healpix"); -static const GridRegisterName GRIDNAME("[hH][1-9][0-9]*"); +static const GridRegisterName GRIDNAME("[hH][0-9]+"); } // namespace eckit::geo::grid diff --git a/tests/geo/grid_healpix.cc b/tests/geo/grid_healpix.cc index 5e13d52a7..e729fcb7e 100644 --- a/tests/geo/grid_healpix.cc +++ b/tests/geo/grid_healpix.cc @@ -57,7 +57,6 @@ CASE("sizes") { CASE("points") { - std::unique_ptr ring(new grid::HEALPix(2)); EXPECT(ring->ordering() == Ordering::healpix_ring); @@ -191,6 +190,13 @@ CASE("equals") { } +CASE("wrong spec") { + EXPECT_THROWS_AS(auto* ignore = GridFactory::make_from_string("{grid:h0}"), exception::SpecError); + EXPECT_THROWS_AS(auto* ignore = GridFactory::make_from_string("{grid:h3, ordering:nested}"), exception::SpecError); + EXPECT_THROWS_AS(auto* ignore = GridFactory::make_from_string("{grid:h3, ordering:?}"), exception::SpecError); +} + + } // namespace eckit::geo::test