Fix lTrim/rTrim asymmetry with trim. Fix readBySeparator on an empty buffer.

Reuse RegEx match data and bound the pattern cache.

lTrim and rTrim returned an all-separator string unchanged where trim() returns empty;
all sixteen overloads now drop everything when no non-separator is left.
readBySeparator handed the callback one empty chunk for an empty buffer; it hands none
now, as the UTF-32 overloads already did.

RegEx::matches allocated its match data per call, four allocations per row while
filtering over a table. It is created once per object now, held by a unique_ptr whose
deleter knows the engine, so the destructor no longer repeats that split. RegEx is
movable but not copyable: it was implicitly copyable while owning that block, and a
copy taken after a match released it twice. syntaxtokenizer.cpp was copying its
std::variant temporary because the user-declared destructor suppressed the implicit
move; it moves now.

RegExCache held two unordered_maps, hash to pattern and hash to options, and never
evicted. It is one DynamicLRU<8192, size_t, shared_ptr<void>> now: the engine lives in
each pattern's deleter so the options map is gone, and values own the pattern, because
an eviction must not free one a live RegEx is still matching with. That also fixes a
racing insert that leaked the loser's pattern. The bound leaves ample room over the
working set and cache hits stay allocation-free (0 new, 0 malloc per 1000
constructions). New tests: lTrimAndRTrim, readBySeparator, splitCb, cacheIsBounded.
Release and AddressSanitizer suites pass 1216 tests each.

include/eepp/system/regex.hpp

@@ -2,31 +2,42 @@
 #define EE_SYSTEM_REGEX
 
 #include <eepp/core/containers.hpp>
+#include <eepp/core/lrucache.hpp>
 #include <eepp/system/mutex.hpp>
 #include <eepp/system/patternmatcher.hpp>
 #include <eepp/system/singleton.hpp>
+#include <memory>
 
 namespace EE { namespace System {
 
 class EE_API RegExCache {
 	SINGLETON_DECLARE_HEADERS( RegExCache )
   public:
-	~RegExCache();
+	/** A compiled pattern, shared by every RegEx that asked for the same pattern and options. The
+	 *  deleter stored inside it releases the pattern with the engine that compiled it, so a pattern
+	 *  stays valid for as long as a RegEx holds it, even after the cache evicts its entry. */
+	using CompiledPattern = std::shared_ptr<void>;
+
+	/** Cached patterns are evicted least-recently-used beyond this count. Syntax definitions are the
+	 *  bulk of the working set (every language contributes a few dozen patterns), so this keeps a
+	 *  whole session's worth resident while bounding the memory the cache can pin. */
+	static constexpr size_t MaxCachedPatterns = 8192;
 
 	bool isEnabled() const { return mEnabled; }
 
 	void setEnabled( bool enabled );
 
-	void insert( std::string_view, Uint32 options, void* cache );
+	void insert( std::string_view pattern, Uint32 options, CompiledPattern compiled );
 
-	void* find( std::string_view, Uint32 options );
+	CompiledPattern find( std::string_view pattern, Uint32 options );
+
+	size_t size();
 
 	void clear();
 
   protected:
 	bool mEnabled{ true };
-	std::unordered_map<size_t, void*> mCache;
-	std::unordered_map<size_t, Uint32> mCacheOpt;
+	DynamicLRU<MaxCachedPatterns, size_t, CompiledPattern> mCache;
 	Mutex mMutex;
 };
 
@@ -73,6 +84,11 @@ class EE_API RegEx : public PatternMatcher {
 	RegEx( std::string_view pattern, Uint32 options = Options::Utf | Options::AllowFallback,
 		   bool useCache = true );
 
+	/** Movable, not copyable: the compiled pattern and the match data are owned. Moving hands them
+	 *  over, copying would release them twice. */
+	RegEx( RegEx&& ) noexcept = default;
+	RegEx& operator=( RegEx&& ) noexcept = default;
+
 	virtual ~RegEx();
 
 	virtual bool isValid() const override { return mValid; }
@@ -80,6 +96,8 @@ class EE_API RegEx : public PatternMatcher {
 	virtual bool matches( const char* stringSearch, int stringStartOffset,
 						  PatternMatcher::Range* matchList, size_t stringLength ) const override;
 
+	/** @note Not thread-safe: the object keeps reusable match state (mMatchNum, mMatchData), so a
+	 *  single instance must not be used from several threads at once. Construct one per thread. */
 	virtual bool matches( const std::string& str, PatternMatcher::Range* matchList = nullptr,
 						  int stringStartOffset = 0 ) const override;
 
@@ -90,15 +108,27 @@ class EE_API RegEx : public PatternMatcher {
 	const std::string_view& getPattern() const override { return mPattern; }
 
   protected:
+	/** Releases match data with the engine that created it. Owning it through this type is what
+	 *  keeps a RegEx move-only: a copy would free the same block twice. */
+	struct MatchDataDeleter {
+		Uint32 options;
+
+		void operator()( void* matchData ) const;
+	};
+
 	std::string_view mPattern;
 	mutable size_t mMatchNum;
-	/** Compiled pattern, owned by the engine named in the Options::UseOniguruma bit of mOptions:
-	 *  pcre2_code* or OnigRegex respectively. The cache owns it when mCached is set. */
-	void* mCompiledPattern;
+	/** Compiled pattern: pcre2_code* or OnigRegex, according to the Options::UseOniguruma bit of
+	 *  mOptions. It owns the pattern and releases it with the engine that compiled it, so the cache
+	 *  may drop its entry while this object is still using the pattern. */
+	RegExCache::CompiledPattern mCompiledPattern;
+	/** Match data reused by every matches() call, opaque so the engine headers stay out of this
+	 *  one: pcre2_match_data* or OnigRegion*, according to the same option bit. It depends only on
+	 *  the compiled pattern, which never changes, so it is created once and owned by this object. */
+	mutable std::unique_ptr<void, MatchDataDeleter> mMatchData;
 	int mCaptureCount{ 0 };
 	Uint32 mOptions{ Options::Utf | Options::AllowFallback };
 	bool mValid : 1 { false };
-	bool mCached : 1 { false };
 	bool mFilterOutCaptures : 1 { false };
 
 	bool initWithOnigumura( std::string_view pattern, bool useCache );

src/eepp/core/string.cpp

@@ -1205,12 +1205,16 @@ std::string String::join( const std::vector<const char*>& strArray, const Int8&
 
 std::string String::lTrim( const std::string& str, char character ) {
 	std::string::size_type pos1 = str.find_first_not_of( character );
-	return ( pos1 == std::string::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 std::string String::rTrim( const std::string& str, char character ) {
 	std::string::size_type pos1 = str.find_last_not_of( character );
-	return ( pos1 == std::string::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 std::string String::trim( const std::string& str, char character ) {
@@ -1224,12 +1228,16 @@ std::string String::trim( const std::string& str, char character ) {
 
 std::string_view String::lTrim( const std::string_view& str, char character ) {
 	std::string::size_type pos1 = str.find_first_not_of( character );
-	return ( pos1 == std::string::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 std::string_view String::rTrim( const std::string_view& str, char character ) {
 	std::string::size_type pos1 = str.find_last_not_of( character );
-	return ( pos1 == std::string::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 std::string_view String::trim( const std::string_view& str, char character ) {
@@ -1243,12 +1251,16 @@ std::string_view String::trim( const std::string_view& str, char character ) {
 
 String::View String::lTrim( const String::View& str, char character ) {
 	String::View::size_type pos1 = str.find_first_not_of( character );
-	return ( pos1 == String::View::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == String::View::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 String::View String::rTrim( const String::View& str, char character ) {
 	String::View::size_type pos1 = str.find_last_not_of( character );
-	return ( pos1 == String::View::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == String::View::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 String::View String::trim( const String::View& str, char character ) {
@@ -1268,12 +1280,16 @@ void String::trimInPlace( std::string& str, char character ) {
 
 String String::lTrim( const String& str, char character ) {
 	StringType::size_type pos1 = str.find_first_not_of( character );
-	return ( pos1 == String::InvalidPos ) ? str : str.substr( pos1 );
+	if ( pos1 == String::InvalidPos )
+		return {};
+	return str.substr( pos1 );
 }
 
 String String::rTrim( const String& str, char character ) {
 	StringType::size_type pos1 = str.find_last_not_of( character );
-	return ( pos1 == String::InvalidPos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == String::InvalidPos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 String String::trim( const String& str, char character ) {
@@ -1291,12 +1307,16 @@ void String::trimInPlace( String& str, char character ) {
 
 std::string String::lTrim( const std::string& str, std::string_view characters ) {
 	std::string::size_type pos1 = str.find_first_not_of( characters );
-	return ( pos1 == std::string::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 std::string String::rTrim( const std::string& str, std::string_view characters ) {
 	std::string::size_type pos1 = str.find_last_not_of( characters );
-	return ( pos1 == std::string::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 std::string String::trim( const std::string& str, std::string_view characters ) {
@@ -1310,12 +1330,16 @@ std::string String::trim( const std::string& str, std::string_view characters )
 
 std::string_view String::lTrim( const std::string_view& str, std::string_view characters ) {
 	std::string::size_type pos1 = str.find_first_not_of( characters );
-	return ( pos1 == std::string::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 std::string_view String::rTrim( const std::string_view& str, std::string_view characters ) {
 	std::string::size_type pos1 = str.find_last_not_of( characters );
-	return ( pos1 == std::string::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == std::string::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 std::string_view String::trim( const std::string_view& str, std::string_view characters ) {
@@ -1329,12 +1353,16 @@ std::string_view String::trim( const std::string_view& str, std::string_view cha
 
 String::View String::lTrim( const String::View& str, String::View characters ) {
 	String::View::size_type pos1 = str.find_first_not_of( characters );
-	return ( pos1 == String::View::npos ) ? str : str.substr( pos1 );
+	if ( pos1 == String::View::npos )
+		return {};
+	return str.substr( pos1 );
 }
 
 String::View String::rTrim( const String::View& str, String::View characters ) {
 	String::View::size_type pos1 = str.find_last_not_of( characters );
-	return ( pos1 == String::View::npos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == String::View::npos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 String::View String::trim( const String::View& str, String::View characters ) {
@@ -1352,12 +1380,16 @@ void String::trimInPlace( std::string& str, std::string_view characters ) {
 
 String String::lTrim( const String& str, std::string_view characters ) {
 	StringType::size_type pos1 = str.find_first_not_of( characters );
-	return ( pos1 == String::InvalidPos ) ? str : str.substr( pos1 );
+	if ( pos1 == String::InvalidPos )
+		return {};
+	return str.substr( pos1 );
 }
 
 String String::rTrim( const String& str, std::string_view characters ) {
 	StringType::size_type pos1 = str.find_last_not_of( characters );
-	return ( pos1 == String::InvalidPos ) ? str : str.substr( 0, pos1 + 1 );
+	if ( pos1 == String::InvalidPos )
+		return {};
+	return str.substr( 0, pos1 + 1 );
 }
 
 String String::trim( const String& str, std::string_view characters ) {
@@ -2696,6 +2728,10 @@ size_t String::toUtf32( std::string_view utf8str, String::StringBaseType* buffer
 
 void String::readBySeparator( std::string_view buf,
 							  std::function<void( std::string_view )> onSepChunkRead, char sep ) {
+	// An empty buffer holds no chunks, so the callback is never handed a spurious empty one.
+	if ( buf.empty() )
+		return;
+
 	auto lastNL = 0;
 	auto nextNL = buf.find_first_of( sep );
 	if ( nextNL != std::string_view::npos ) {
@@ -2716,6 +2752,10 @@ void String::readBySeparator( std::string_view buf,
 void String::readBySeparatorStoppable( std::string_view buf,
 									   std::function<bool( std::string_view )> onSepChunkRead,
 									   char sep ) {
+	// An empty buffer holds no chunks, so the callback is never handed a spurious empty one.
+	if ( buf.empty() )
+		return;
+
 	auto lastNL = 0;
 	auto nextNL = buf.find_first_of( sep );
 	if ( nextNL != std::string_view::npos ) {

src/eepp/system/regex.cpp

@@ -15,42 +15,48 @@ struct OnigInitializer {
 	~OnigInitializer() { onig_end(); }
 };
 
+/** Releases a compiled pattern with the engine that produced it. The cache stores one of these
+ *  inside every shared_ptr it hands out, which is what lets a RegEx keep using a pattern after the
+ *  cache has evicted the entry. */
+struct CompiledPatternDeleter {
+	Uint32 options;
+
+	void operator()( void* pattern ) const {
+		if ( options & RegEx::Options::UseOniguruma )
+			onig_free( static_cast<OnigRegex>( pattern ) );
+		else
+			pcre2_code_free( static_cast<pcre2_code*>( pattern ) );
+	}
+};
+
 static OnigInitializer globalOnigInitializer;
 
 } // namespace
 
 SINGLETON_DECLARE_IMPLEMENTATION( RegExCache )
 
-RegExCache::~RegExCache() {
-	clear();
-}
-
 inline size_t getCacheHash( std::string_view key, Uint32 options ) {
 	return hashCombine( std::hash<std::string_view>()( key ), options );
 }
 
-void RegExCache::insert( std::string_view key, Uint32 options, void* cache ) {
-	auto hash = getCacheHash( key, options );
+void RegExCache::insert( std::string_view pattern, Uint32 options, CompiledPattern compiled ) {
 	Lock l( mMutex );
-	mCache.insert( { hash, cache } );
-	mCacheOpt.insert( { hash, options } );
+	mCache.put( getCacheHash( pattern, options ), std::move( compiled ) );
 }
 
-void* RegExCache::find( std::string_view key, Uint32 options ) {
+RegExCache::CompiledPattern RegExCache::find( std::string_view pattern, Uint32 options ) {
 	Lock l( mMutex );
-	auto it = mCache.find( getCacheHash( key, options ) );
-	return ( it != mCache.end() ) ? it->second : nullptr;
+	auto cached = mCache.get( getCacheHash( pattern, options ) );
+	return cached ? std::move( *cached ) : CompiledPattern();
+}
+
+size_t RegExCache::size() {
+	Lock l( mMutex );
+	return mCache.size();
 }
 
 void RegExCache::clear() {
 	Lock l( mMutex );
-	for ( auto& cache : mCache ) {
-		auto opt = mCacheOpt.find( cache.first );
-		if ( opt->second & RegEx::Options::UseOniguruma )
-			onig_free( static_cast<OnigRegex>( cache.second ) );
-		else
-			pcre2_code_free( reinterpret_cast<pcre2_code*>( cache.second ) );
-	}
 	mCache.clear();
 }
 
@@ -58,7 +64,6 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) :
 	PatternMatcher( PatternType::PCRE ),
 	mPattern( pattern ),
 	mMatchNum( 0 ),
-	mCompiledPattern( nullptr ),
 	mCaptureCount( 0 ),
 	mOptions( options ),
 	mValid( true ),
@@ -70,7 +75,6 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) :
 	if ( useCache && RegExCache::instance()->isEnabled() &&
 		 ( mCompiledPattern = RegExCache::instance()->find( pattern, mOptions ) ) ) {
 		mValid = true;
-		mCached = true;
 		return;
 	}
 
@@ -79,7 +83,6 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) :
 		 ( mCompiledPattern =
 			   RegExCache::instance()->find( pattern, mOptions | Options::UseOniguruma ) ) ) {
 		mValid = true;
-		mCached = true;
 		mOptions |= Options::UseOniguruma;
 		return;
 	}
@@ -98,15 +101,15 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) :
 	if ( options & Options::UseOniguruma )
 		options &= ~Options::UseOniguruma;
 
-	mCompiledPattern = pcre2_compile( pattern_sptr,	  // the pattern
-									  pattern.size(), // the length of the pattern
-									  options,		  // default options
-									  &errornumber,	  // for error number
-									  &erroroffset,	  // for error offset
-									  NULL			  // use default compile context
+	auto* compiled = pcre2_compile( pattern_sptr,	// the pattern
+									pattern.size(), // the length of the pattern
+									options,		// default options
+									&errornumber,	// for error number
+									&erroroffset,	// for error offset
+									NULL			// use default compile context
 	);
 
-	if ( mCompiledPattern == NULL ) {
+	if ( compiled == NULL ) {
 		PCRE2_UCHAR buffer[256];
 		pcre2_get_error_message( errornumber, buffer, sizeof( buffer ) );
 		mValid = false;
@@ -119,34 +122,32 @@ RegEx::RegEx( std::string_view pattern, Uint32 options, bool useCache ) :
 		return;
 	}
 
+	mCompiledPattern =
+		RegExCache::CompiledPattern( compiled, CompiledPatternDeleter{ mOptions } );
+
 #if EE_PLATFORM != EE_PLATFORM_EMSCRIPTEN
-	pcre2_jit_compile( reinterpret_cast<pcre2_code*>( mCompiledPattern ), PCRE2_JIT_COMPLETE );
+	pcre2_jit_compile( static_cast<pcre2_code*>( mCompiledPattern.get() ), PCRE2_JIT_COMPLETE );
 #endif
 
-	int rc = pcre2_pattern_info( reinterpret_cast<pcre2_code*>( mCompiledPattern ),
+	int rc = pcre2_pattern_info( static_cast<pcre2_code*>( mCompiledPattern.get() ),
 								 PCRE2_INFO_CAPTURECOUNT, &mCaptureCount );
 	if ( rc != 0 ) {
 		Log::debug( "PCRE2 pattern info failed with error code " + std::to_string( rc ) );
 		mValid = false;
 	} else if ( useCache && RegExCache::instance()->isEnabled() ) {
 		RegExCache::instance()->insert( pattern, mOptions, mCompiledPattern );
-		mCached = true;
 	}
 }
 
-RegEx::~RegEx() {
-	if ( mCached || mCompiledPattern == nullptr )
-		return;
-
-	// The pattern is owned by whichever engine compiled it, so it must be released with that
-	// engine's deallocator: freeing an Oniguruma pattern with pcre2_code_free() (or the reverse)
-	// corrupts the heap. The cache, which owns the patterns it hands out, does the same split.
-	if ( mOptions & Options::UseOniguruma )
-		onig_free( static_cast<OnigRegex>( mCompiledPattern ) );
+void RegEx::MatchDataDeleter::operator()( void* matchData ) const {
+	if ( options & Options::UseOniguruma )
+		onig_region_free( static_cast<OnigRegion*>( matchData ), 1 );
 	else
-		pcre2_code_free( reinterpret_cast<pcre2_code*>( mCompiledPattern ) );
+		pcre2_match_data_free( static_cast<pcre2_match_data*>( matchData ) );
 }
 
+RegEx::~RegEx() = default;
+
 bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 					 PatternMatcher::Range* matchList, size_t stringLength ) const {
 	if ( !mValid || !mCompiledPattern ) {
@@ -155,7 +156,10 @@ bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 	}
 
 	if ( mOptions & Options::UseOniguruma ) {
-		OnigRegion* region = onig_region_new();
+		if ( !mMatchData )
+			mMatchData = std::unique_ptr<void, MatchDataDeleter>( onig_region_new(),
+																  MatchDataDeleter{ mOptions } );
+		OnigRegion* region = static_cast<OnigRegion*>( mMatchData.get() );
 		if ( !region ) {
 			Log::error( "Onigumura: onig_region_new() failed." );
 			mMatchNum = 0;
@@ -169,15 +173,14 @@ bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 		OnigOptionType searchOpt = ONIG_OPTION_NONE;
 
 		if ( stringStartOffset > static_cast<int>( stringLength ) ) {
-			onig_region_free( region, 1 );
 			mMatchNum = 0;
 			return false;
 		}
 
 		int ret = ( mOptions & Options::Anchored )
-					  ? onig_match( static_cast<OnigRegex>( mCompiledPattern ), subjectPtr,
+					  ? onig_match( static_cast<OnigRegex>( mCompiledPattern.get() ), subjectPtr,
 									subjectEnd, subjectStart, region, searchOpt )
-					  : onig_search( static_cast<OnigRegex>( mCompiledPattern ), subjectPtr,
+					  : onig_search( static_cast<OnigRegex>( mCompiledPattern.get() ), subjectPtr,
 									 subjectEnd, subjectStart, subjectEnd, region, searchOpt );
 
 		if ( ret >= 0 ) {
@@ -203,25 +206,33 @@ bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 					mMatchNum = curCap;
 			}
 
-			onig_region_free( region, 1 );
 			return mMatchNum > 0;
 
 		} else if ( ret == ONIG_MISMATCH ) { // No match
-			onig_region_free( region, 1 );
 			mMatchNum = 0;
 			return false;
 		} else { // Error
 			UChar errBuf[ONIG_MAX_ERROR_MESSAGE_LEN];
 			onig_error_code_to_str( errBuf, ret );
 			Log::debug( "Onigumura search error: %s", reinterpret_cast<const char*>( errBuf ) );
-			onig_region_free( region, 1 );
 			mMatchNum = 0;
 			return false;
 		}
 	}
 
-	auto* compiledPattern = reinterpret_cast<pcre2_code*>( mCompiledPattern );
-	pcre2_match_data* match_data = pcre2_match_data_create_from_pattern( compiledPattern, NULL );
+	auto* compiledPattern = static_cast<pcre2_code*>( mCompiledPattern.get() );
+
+	// The ovector size is taken from the compiled pattern, so one match data block serves every
+	// call this object ever makes.
+	if ( !mMatchData )
+		mMatchData = std::unique_ptr<void, MatchDataDeleter>(
+			pcre2_match_data_create_from_pattern( compiledPattern, NULL ),
+			MatchDataDeleter{ mOptions } );
+	pcre2_match_data* match_data = static_cast<pcre2_match_data*>( mMatchData.get() );
+	if ( match_data == nullptr ) {
+		mMatchNum = 0;
+		return false;
+	}
 
 	PCRE2_SPTR subject = reinterpret_cast<PCRE2_SPTR>( stringSearch );
 
@@ -235,7 +246,6 @@ bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 	);
 
 	if ( rc < 0 ) {
-		pcre2_match_data_free( match_data );
 		mMatchNum = 0;
 		// if ( rc == PCRE2_ERROR_NOMATCH )
 		return false;
@@ -264,7 +274,6 @@ bool RegEx::matches( const char* stringSearch, int stringStartOffset,
 		mMatchNum = curCap;
 	}
 
-	pcre2_match_data_free( match_data );
 	return mMatchNum > 0;
 }
 
@@ -281,9 +290,9 @@ int RegEx::getCaptureCount() const {
 	if ( !mCompiledPattern )
 		return 0;
 	if ( mOptions & Options::UseOniguruma )
-		return onig_number_of_captures( static_cast<OnigRegex>( mCompiledPattern ) );
+		return onig_number_of_captures( static_cast<OnigRegex>( mCompiledPattern.get() ) );
 	int captureCount = 0;
-	return pcre2_pattern_info( reinterpret_cast<pcre2_code*>( mCompiledPattern ),
+	return pcre2_pattern_info( static_cast<pcre2_code*>( mCompiledPattern.get() ),
 							   PCRE2_INFO_CAPTURECOUNT, &captureCount ) == 0
 			   ? captureCount
 			   : 0;
@@ -310,22 +319,18 @@ bool RegEx::initWithOnigumura( std::string_view pattern, bool useCache ) {
 		UChar errBuf[ONIG_MAX_ERROR_MESSAGE_LEN];
 		onig_error_code_to_str( errBuf, ret, &err );
 		Log::info( "Onigumura compilation failed: %s", reinterpret_cast<const char*>( errBuf ) );
+		// onig_new() failed, so there is no pattern to release: mCompiledPattern is left null.
 		mValid = false;
-		if ( mCompiledPattern ) {
-			onig_free( regex );
-			mCompiledPattern = nullptr;
-		}
 		return false;
 	}
 
-	mCompiledPattern = regex;
-	mValid = true;
 	mOptions |= Options::UseOniguruma;
-	mCaptureCount = onig_number_of_captures( static_cast<OnigRegex>( mCompiledPattern ) );
+	mCompiledPattern = RegExCache::CompiledPattern( regex, CompiledPatternDeleter{ mOptions } );
+	mValid = true;
+	mCaptureCount = onig_number_of_captures( static_cast<OnigRegex>( mCompiledPattern.get() ) );
 
 	if ( useCache && RegExCache::instance()->isEnabled() ) {
 		RegExCache::instance()->insert( pattern, mOptions, mCompiledPattern );
-		mCached = true;
 	}
 
 	return false;

src/tests/unit_tests/regex_tests.cpp

@@ -40,6 +40,44 @@ UTEST( RegEx, cacheHit ) {
 	RegExCache::destroySingleton();
 }
 
+UTEST( RegEx, cacheIsBounded ) {
+	// The cache is bounded and evicts the least recently used pattern. A pattern it drops has to
+	// stay valid for the RegEx still using it: patterns are shared with the cache, never borrowed.
+	RegExCache::destroySingleton();
+	const Uint32 options = RegEx::Options::Utf | RegEx::Options::AllowFallback;
+	const std::string subject( "evictme" );
+	RegEx evicted( subject, options );
+	EXPECT_TRUE( evicted.matches( subject ) );
+	EXPECT_TRUE( RegExCache::instance()->find( subject, options ) != nullptr );
+
+	// Filling the bound with placeholders is far cheaper than compiling that many patterns. The
+	// no-op deleter is safe because these are not engine patterns.
+	auto placeholder = std::shared_ptr<void>( reinterpret_cast<void*>( 1 ), []( void* ) {} );
+	auto insert = [&]( const std::string& key ) {
+		RegExCache::instance()->insert( key, options, placeholder );
+	};
+	for ( size_t i = 0; i < RegExCache::MaxCachedPatterns; ++i )
+		insert( "placeholder" + std::to_string( i ) );
+
+	EXPECT_TRUE( RegExCache::instance()->size() <= RegExCache::MaxCachedPatterns );
+	EXPECT_TRUE( RegExCache::instance()->find( subject, options ) == nullptr );
+	EXPECT_TRUE( evicted.matches( subject ) );
+	EXPECT_EQ( evicted.getNumMatches(), 1ul );
+
+	// `touched` goes in first, so it is the least recently used entry once the cache fills again.
+	// The lookup promotes it, which means the insert that follows has to drop `filler0` instead.
+	const std::string touched = "touched";
+	insert( touched );
+	for ( size_t i = 0; i < RegExCache::MaxCachedPatterns - 1; ++i )
+		insert( "filler" + std::to_string( i ) );
+	EXPECT_TRUE( RegExCache::instance()->find( touched, options ) != nullptr );
+	insert( "extra" );
+	EXPECT_TRUE( RegExCache::instance()->find( touched, options ) != nullptr );
+	EXPECT_TRUE( RegExCache::instance()->find( "filler0", options ) == nullptr );
+
+	RegExCache::destroySingleton();
+}
+
 UTEST( RegEx, captures ) {
 	RegEx regex( "(\\d+) and (\\d+)" );
 	EXPECT_EQ( regex.getCaptureCount(), 2 );
@@ -149,10 +187,9 @@ UTEST( RegExEngines, basicTest ) {
 }
 
 UTEST( RegExEngines, uncachedPatternIsFreedByItsOwnEngine ) {
-	// A pattern compiled by Oniguruma but not owned by the cache has to be released with onig_free().
-	// Releasing it with pcre2_code_free() corrupted the heap and crashed this test binary, so the
-	// engine that compiled a pattern decides its deallocator (the same split RegExCache::clear()
-	// makes for the patterns it owns).
+	// A pattern compiled by Oniguruma has to be released with onig_free(). Releasing it with
+	// pcre2_code_free() corrupted the heap and crashed this test binary, so every compiled pattern
+	// carries the deallocator of the engine that produced it, cached or not.
 	{
 		RegEx oniguruma( "a+", RegEx::Options::Utf | RegEx::Options::UseOniguruma, false );
 		EXPECT_EQ( oniguruma.isValid(), true );

src/tests/unit_tests/stringsoperations_tests.cpp

@@ -4,6 +4,8 @@
 #include <eepp/system/filesystem.hpp>
 #include <eepp/system/sys.hpp>
 #include <filesystem>
+#include <string_view>
+#include <vector>
 
 using namespace std::literals;
 
@@ -134,6 +136,163 @@ UTEST( String, trim ) {
 				 String::View( U"a" ) );
 }
 
+UTEST( String, lTrimAndRTrim ) {
+	// Only the requested side is removed and interior separators are kept.
+	EXPECT_TRUE( String::lTrim( std::string( "  a  " ) ) == std::string( "a  " ) );
+	EXPECT_TRUE( String::rTrim( std::string( "  a  " ) ) == std::string( "  a" ) );
+	EXPECT_TRUE( String::lTrim( std::string( "abc" ) ) == std::string( "abc" ) );
+	EXPECT_TRUE( String::rTrim( std::string( "abc" ) ) == std::string( "abc" ) );
+	EXPECT_TRUE( String::lTrim( std::string( "xxa" ), 'x' ) == std::string( "a" ) );
+	EXPECT_TRUE( String::rTrim( std::string( "axx" ), 'x' ) == std::string( "a" ) );
+	EXPECT_TRUE( String::lTrim( std::string( "\t a " ), std::string_view( " \t" ) ) ==
+				 std::string( "a " ) );
+
+	// A string made only of separators has nothing left, as in trim().
+	EXPECT_TRUE( String::lTrim( std::string() ).empty() );
+	EXPECT_TRUE( String::rTrim( std::string() ).empty() );
+	EXPECT_TRUE( String::lTrim( std::string( "   " ) ).empty() );
+	EXPECT_TRUE( String::rTrim( std::string( "   " ) ).empty() );
+	EXPECT_TRUE( String::lTrim( std::string( "xxxx" ), 'x' ).empty() );
+	EXPECT_TRUE( String::rTrim( std::string( "xxxx" ), 'x' ).empty() );
+	EXPECT_TRUE( String::lTrim( std::string( " \t\n " ), std::string_view( " \t\n" ) ).empty() );
+	EXPECT_TRUE( String::rTrim( std::string( " \t\n " ), std::string_view( " \t\n" ) ).empty() );
+	EXPECT_TRUE( String::lTrim( std::string_view( "   " ) ).empty() );
+	EXPECT_TRUE( String::rTrim( std::string_view( "   " ) ).empty() );
+
+	// The UTF-32 overloads are separate implementations.
+	EXPECT_TRUE( String::lTrim( String( "   " ) ).empty() );
+	EXPECT_TRUE( String::rTrim( String( "   " ) ).empty() );
+	EXPECT_TRUE( String::lTrim( String( "  a  " ) ) == String( "a  " ) );
+	EXPECT_TRUE( String::rTrim( String( "  a  " ) ) == String( "  a" ) );
+	EXPECT_TRUE( String::lTrim( String::View( U"   " ) ).empty() );
+	EXPECT_TRUE( String::rTrim( String::View( U"   " ) ).empty() );
+	EXPECT_TRUE( String::lTrim( String::View( U"  a  " ) ) == String::View( U"a  " ) );
+	EXPECT_TRUE( String::rTrim( String::View( U"  a  " ) ) == String::View( U"  a" ) );
+
+	// Trimming one side and then the other is what trim() does in one step.
+	EXPECT_TRUE( String::rTrim( String::lTrim( std::string( "  a b  " ) ) ) ==
+				 String::trim( std::string( "  a b  " ) ) );
+}
+
+UTEST( String, readBySeparator ) {
+	auto collect = []( const std::string& input, char sep ) {
+		std::vector<std::string> chunks;
+		String::readBySeparator(
+			input, [&]( std::string_view chunk ) { chunks.emplace_back( chunk ); }, sep );
+		return chunks;
+	};
+
+	// An empty buffer holds no chunks, so the callback is not handed a spurious empty one.
+	EXPECT_TRUE( collect( std::string(), '\n' ).empty() );
+
+	// A buffer without a separator is a single chunk.
+	{
+		auto chunks = collect( "abc", '\n' );
+		EXPECT_EQ( chunks.size(), 1ul );
+		EXPECT_TRUE( chunks[0] == std::string( "abc" ) );
+	}
+
+	// A trailing separator does not add an empty chunk.
+	{
+		auto chunks = collect( "a\n", '\n' );
+		EXPECT_EQ( chunks.size(), 1ul );
+		EXPECT_TRUE( chunks[0] == std::string( "a" ) );
+	}
+
+	// Empty lines between separators are preserved, and a lone separator is one empty chunk.
+	{
+		auto chunks = collect( "a\n\nb", '\n' );
+		EXPECT_EQ( chunks.size(), 3ul );
+		EXPECT_TRUE( chunks[0] == std::string( "a" ) );
+		EXPECT_TRUE( chunks[1].empty() );
+		EXPECT_TRUE( chunks[2] == std::string( "b" ) );
+	}
+	EXPECT_EQ( collect( "\n", '\n' ).size(), 1ul );
+
+	// The separator is configurable.
+	{
+		auto chunks = collect( "a;b;", ';' );
+		EXPECT_EQ( chunks.size(), 2ul );
+		EXPECT_TRUE( chunks[0] == std::string( "a" ) );
+		EXPECT_TRUE( chunks[1] == std::string( "b" ) );
+	}
+
+	// The stoppable variant stops at the first chunk that asks it to, and skips empty buffers.
+	{
+		int seen = 0;
+		String::readBySeparatorStoppable( std::string( "a\nb\nc" ), [&]( std::string_view ) {
+			++seen;
+			return true;
+		} );
+		EXPECT_EQ( seen, 1 );
+
+		seen = 0;
+		String::readBySeparatorStoppable( std::string(), [&]( std::string_view ) {
+			++seen;
+			return false;
+		} );
+		EXPECT_EQ( seen, 0 );
+	}
+}
+
+UTEST( String, splitCb ) {
+	auto split = []( const std::string& input, const std::string& delims,
+					 const std::string& preserve = "", const std::string& quote = "\"",
+					 bool removeQuotes = false ) {
+		std::vector<std::string> tokens;
+		String::splitCb(
+			[&]( std::string_view token ) {
+				tokens.emplace_back( token );
+				return true;
+			},
+			input, delims, preserve, quote, removeQuotes );
+		return tokens;
+	};
+
+	// Tokens are split on any of the delimiter characters, and empty ones are dropped.
+	{
+		auto tokens = split( "a,b,c", "," );
+		EXPECT_EQ( tokens.size(), 3ul );
+		EXPECT_TRUE( tokens[0] == std::string( "a" ) );
+		EXPECT_TRUE( tokens[2] == std::string( "c" ) );
+	}
+	EXPECT_EQ( split( "a,,c", "," ).size(), 2ul );
+
+	// A buffer with no delimiter is one token, an empty buffer yields none.
+	EXPECT_EQ( split( "abc", "," ).size(), 1ul );
+	EXPECT_TRUE( split( "", "," ).empty() );
+
+	// A quoted token keeps its quotes unless removeQuotes is requested.
+	{
+		auto kept = split( "\"a\",\"b\"", "," );
+		EXPECT_EQ( kept.size(), 2ul );
+		EXPECT_TRUE( kept[0] == std::string( "\"a\"" ) );
+
+		auto stripped = split( "\"a\",\"b\"", ",", "", "\"", true );
+		EXPECT_EQ( stripped.size(), 2ul );
+		EXPECT_TRUE( stripped[0] == std::string( "a" ) );
+		EXPECT_TRUE( stripped[1] == std::string( "b" ) );
+	}
+
+	// delimsPreserve hands the preserved separator over as a token of its own.
+	{
+		auto tokens = split( "a;b", "", ";" );
+		EXPECT_EQ( tokens.size(), 3ul );
+		EXPECT_TRUE( tokens[0] == std::string( "a" ) );
+		EXPECT_TRUE( tokens[1] == std::string( ";" ) );
+		EXPECT_TRUE( tokens[2] == std::string( "b" ) );
+	}
+
+	// Brackets group only when they are part of the quote set, which is what code splitting needs.
+	EXPECT_EQ( split( "f(a,b),c", "," ).size(), 3ul );
+	{
+		auto tokens = split( "f(a,b),c", ",", "", "(" );
+		EXPECT_EQ( tokens.size(), 2ul );
+		EXPECT_TRUE( tokens[0] == std::string( "f(a,b)" ) );
+		EXPECT_TRUE( tokens[1] == std::string( "c" ) );
+	}
+}
+
 UTEST( String, reusableFormattingAndUtf8Assignment ) {
 	std::string formatted;
 	formatted.reserve( 128 );