From d45cb911dd3853bc5f590d12265706aeaa9f0080 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Mon, 18 Mar 2019 18:34:39 +0100 Subject: Remove broken MergeDataHashMap optimization ...which apparently didn't take into account that inserting into a std::unordered_map may invalidate iterators, which now started to cause build failures like > [PRP] dictionaries/hu_HU/dialog/hu_HU_de > warn:sal.osl:25175:25175:sal/osl/unx/thread.cxx:1026: RTL_TEXTENCODING_DONTKNOW -> _ASCII_US > .../gcc/trunk/inst/include/c++/9.0.1/debug/safe_iterator.h:307: > In function: > __gnu_debug::_Safe_iterator<_Iterator, _Sequence, _Category>::pointer > __gnu_debug::_Safe_iterator<_Iterator, _Sequence, > _Category>::operator->() const [with _Iterator = > std::__detail::_Node_iterator std::unique_ptr >, false, true>; _Sequence = > std::__debug::unordered_map >; > _Category = std::forward_iterator_tag; > __gnu_debug::_Safe_iterator<_Iterator, _Sequence, _Category>::pointer = > std::pair >*] > > Error: attempt to dereference a singular iterator. > > Objects involved in the operation: > iterator "this" @ 0x0x5a8228 { > type = std::__detail::_Node_iterator > >, false, true> (mutable iterator); > state = singular; > references sequence with type 'std::__debug::unordered_map >, std::hash, std::equal_to, std::allocator > > > >' @ 0x0x5a81c8 > } > /bin/sh: line 1: 25175 Aborted (core dumped) LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}"$I/program:$I/program" $W/LinkTarget/Executable/propex -i $S/dictionaries/hu_HU/dialog/hu_HU_en_US.properties -l de -m ${MERGEINPUT} -o $W/PropertiesTranslateTarget/dictionaries/hu_HU/dialog/hu_HU_de.properties > make[1]: *** [.../solenv/gbuild/Dictionary.mk:88: .../workdir/PropertiesTranslateTarget/dictionaries/hu_HU/dialog/hu_HU_de.properties] Error 134 with trunk libstdc++ in debug mode. Both classes MergeData and MergeDataHashMap became redundant. Reviewed-on: https://gerrit.libreoffice.org/69395 Tested-by: Jenkins Reviewed-by: Stephan Bergmann (cherry picked from commit dc2329fb66dc38f62a21b1afaca38529d7e40fa9) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/85866 Tested-by: Jenkins CollaboraOffice Reviewed-by: Aron Budea Tested-by: Aron Budea (cherry picked from commit 0df20a65441cacb58cddfa6e5451691088bfcf8f) Change-Id: I771548a6155e14037d84acfd74cd13566a26a8d7 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/85962 Tested-by: Jenkins CollaboraOffice Reviewed-by: Andras Timar --- l10ntools/inc/export.hxx | 74 +--------------------------------- l10ntools/source/merge.cxx | 99 ++++++---------------------------------------- 2 files changed, 13 insertions(+), 160 deletions(-) diff --git a/l10ntools/inc/export.hxx b/l10ntools/inc/export.hxx index 0ed0f45c5175..ced715071f2e 100644 --- a/l10ntools/inc/export.hxx +++ b/l10ntools/inc/export.hxx @@ -130,76 +130,6 @@ public: }; - -// class MergeDataHashMap - - -class MergeData; - -/** Container for MergeData - - This class is an HashMap with a hidden insertion - order. The class can used just like a simple - HashMap, but good to know that it's use is - more effective if the accessing(find) order - match with the insertion order. - - In the most case, this match is good. - (e.g. reading PO files of different languages, - executables merging) -*/ -class MergeDataHashMap -{ - private: - typedef std::unordered_map HashMap_t; - - public: - MergeDataHashMap() - : bFirstSearch(true) - , aLastInsertion(m_aHashMap.end()) - , aLastFound(m_aHashMap.end()) - , aFirstInOrder(m_aHashMap.end()) - { - } - - typedef HashMap_t::iterator iterator; - typedef HashMap_t::const_iterator const_iterator; - - std::pair insert(const OString& rKey, MergeData* pMergeData); - iterator const & find(const OString& rKey); - - iterator begin() {return m_aHashMap.begin();} - iterator end() {return m_aHashMap.end();} - - private: - bool bFirstSearch; - HashMap_t m_aHashMap; - iterator aLastInsertion; - iterator aLastFound; - iterator aFirstInOrder; -}; - - -// class MergeData - - -/// Purpose: holds information of data to merge (one resource) -class MergeData -{ - friend class MergeDataHashMap; - -public: - std::unique_ptr pMergeEntrys; -private: - MergeDataHashMap::iterator m_aNextData; -public: - MergeData(); - ~MergeData(); - MergeEntrys* GetMergeEntries() { return pMergeEntrys.get();} - -}; - - // class MergeDataFile @@ -207,10 +137,10 @@ public: class MergeDataFile { private: - MergeDataHashMap aMap; + std::unordered_map> aMap; std::set aLanguageSet; - MergeData *GetMergeData( ResData *pResData , bool bCaseSensitive = false ); + MergeEntrys *GetMergeData( ResData *pResData , bool bCaseSensitive = false ); void InsertEntry(const OString &rTYP, const OString &rGID, const OString &rLID, const OString &nLang, const OString &rTEXT, const OString &rQHTEXT, diff --git a/l10ntools/source/merge.cxx b/l10ntools/source/merge.cxx index a86ddf96124e..ea236948b8b8 100644 --- a/l10ntools/source/merge.cxx +++ b/l10ntools/source/merge.cxx @@ -107,74 +107,6 @@ OString MergeEntrys::GetQTZText(const ResData& rResData, const OString& rOrigTex return sKey + GetDoubleBars() + rOrigText; } - -// class MergeDataHashMap - - -std::pair MergeDataHashMap::insert(const OString& rKey, MergeData* pMergeData) -{ - std::pair aTemp = m_aHashMap.emplace( rKey, pMergeData ); - if( m_aHashMap.size() == 1 ) - { - // When first insert, set an iterator to the first element - aFirstInOrder = aTemp.first; - } - else - { - // Define insertion order by setting an iterator to the next element. - aLastInsertion->second->m_aNextData = aTemp.first; - } - aLastInsertion = aTemp.first; - return aTemp; -} - -MergeDataHashMap::iterator const & MergeDataHashMap::find(const OString& rKey) -{ - iterator aHint = m_aHashMap.end(); - - // Add a hint - if( bFirstSearch && !m_aHashMap.empty() ) - { - aHint = aFirstInOrder; - } - else if( aLastFound == aLastInsertion ) - { - // Next to the last element is the first element - aHint = aFirstInOrder; - } - else if( aLastFound != m_aHashMap.end() && aLastFound != aLastInsertion ) - { - aHint = aLastFound->second->m_aNextData; - } - - // If hint works than no need for search - if( aHint != m_aHashMap.end() && aHint->first == rKey ) - { - aLastFound = aHint; - } - else - { - aLastFound = m_aHashMap.find(rKey); - } - - bFirstSearch = false; - return aLastFound; -} - - -// class MergeData - - -MergeData::MergeData() - : pMergeEntrys( new MergeEntrys() ) -{ -} - -MergeData::~MergeData() -{ -} - - // class MergeDataFile @@ -296,8 +228,6 @@ MergeDataFile::MergeDataFile( MergeDataFile::~MergeDataFile() { - for (MergeDataHashMap::iterator aI = aMap.begin(), aEnd = aMap.end(); aI != aEnd; ++aI) - delete aI->second; } std::vector MergeDataFile::GetLanguages() const @@ -305,7 +235,7 @@ std::vector MergeDataFile::GetLanguages() const return std::vector(aLanguageSet.begin(),aLanguageSet.end()); } -MergeData *MergeDataFile::GetMergeData( ResData *pResData , bool bCaseSensitive ) +MergeEntrys *MergeDataFile::GetMergeData( ResData *pResData , bool bCaseSensitive ) { OString sOldG = pResData->sGId; OString sOldL = pResData->sId; @@ -320,12 +250,12 @@ MergeData *MergeDataFile::GetMergeData( ResData *pResData , bool bCaseSensitive OString sKey = CreateKey( pResData->sResTyp , pResData->sGId , pResData->sId , pResData->sFilename , bCaseSensitive ); - MergeDataHashMap::const_iterator mit = aMap.find( sKey ); + auto mit = aMap.find( sKey ); if(mit != aMap.end()) { pResData->sGId = sOldG; pResData->sId = sOldL; - return mit->second; + return mit->second.get(); } pResData->sGId = sOldG; pResData->sId = sOldL; @@ -335,19 +265,13 @@ MergeData *MergeDataFile::GetMergeData( ResData *pResData , bool bCaseSensitive MergeEntrys *MergeDataFile::GetMergeEntrys( ResData *pResData ) { // search for requested MergeEntrys - MergeData *pData = GetMergeData( pResData ); - if ( pData ) - return pData->GetMergeEntries(); - return nullptr; + return GetMergeData( pResData ); } MergeEntrys *MergeDataFile::GetMergeEntrysCaseSensitive( ResData *pResData ) { // search for requested MergeEntrys - MergeData *pData = GetMergeData( pResData , true ); - if ( pData ) - return pData->GetMergeEntries(); - return nullptr; + return GetMergeData( pResData , true ); } void MergeDataFile::InsertEntry( @@ -357,28 +281,27 @@ void MergeDataFile::InsertEntry( const OString &rTITLE, const OString &rInFilename, bool bFirstLang, bool bCaseSensitive ) { - MergeData *pData = nullptr; + MergeEntrys *pMergeEntrys = nullptr; // search for MergeData OString sKey = CreateKey(rTYP , rGID , rLID , rInFilename , bCaseSensitive); if( !bFirstLang ) { - MergeDataHashMap::const_iterator mit = aMap.find( sKey ); + auto mit = aMap.find( sKey ); if(mit != aMap.end()) - pData = mit->second; + pMergeEntrys = mit->second.get(); } - if( !pData ) + if( !pMergeEntrys ) { - pData = new MergeData; - aMap.insert( sKey, pData ); + pMergeEntrys = new MergeEntrys; + aMap.emplace( sKey, std::unique_ptr(pMergeEntrys) ); } // insert the cur string - MergeEntrys *pMergeEntrys = pData->GetMergeEntries(); if( nLANG =="qtz" ) { const OString sTemp = rInFilename + rGID + rLID + rTYP; -- cgit v1.2.3