From: Robert Wilhelm (robert.wilhelm@gmx.net)
Date: Thu Oct 09 2003 - 16:56:17 EDT
Frank,
thanks a lot for your valuable comments.
I hope to create an updated patch sometime this weekend.
I have so far only converted PP_AttrProp::m_pAttributes
but not yet PP_AttrProp::m_pProperties, which is the
one which appears on top of profiling runs of AbiWord.
BTW in the later case key is XML_Char* und value is UT_Pair *.
Shall I create a subclass of UT_GenericUTF8Hash?
Robert
On Thu, 2003-10-09 at 12:56, Francis James Franklin wrote:
> I know you haven't finished yet (and I'm really curious to see whether
> the class profiles well or not, so thanks for doing this) but I have
> some questions/comments/concerns:
>
> 1.The section from @@ -600,29 +583,38 @ seems to be a HashMap equality
> check - it may be worth writing a UT_UTF8Hash operator==() for this - I
> may do that. But my question is really about the use of strcmp()
> instead of the UT_UTF8String operator==() which ought to perform
> better...?
>
> 2. I'm a little concerned about this part of the patch:
>
> @@ -771,16 +763,24 @@
> if(!m_pAttributes)
> return;
>
> - UT_StringPtrMap::UT_Cursor _hc1(m_pAttributes);
> + unsigned int ui = 0;
> +
> const XML_Char * pEntry;
>
> - for ( pEntry = static_cast<const XML_Char*>(_hc1.first());
> _hc1.is_valid(); pEntry = static_cast<const XML_Char*>(_hc1.next()) )
> + for ( ui = 0; ui < m_pAttributes->count(); ui++)
> {
> + const UT_UTF8String* key1;
> + const UT_UTF8String* value1;
> +
> + m_pAttributes->pair( ui, key1, value1);
> +
> + pEntry = value1->utf8_str();
> +
> if (pEntry && !*pEntry)
> {
> UT_ASSERT(!m_bIsReadOnly);
> FREEP(pEntry);
> - m_pAttributes->remove(_hc1.key(),pEntry);
> + m_pAttributes->del(*key1);
> }
> }
> }
>
> a. The FREEP(pEntry) is criminal.
> b. This use of a for(;;) loop means that you will skip an entry
> whenever you delete one.
> c. This is just stripping values which have zero length? It may be
> worth adding an option to UT_UTF8Hash itself to strip zero-length
> values automatically...?
>
> 3. cch = UT_XML_strlen(s1) should really be cch = key->byteLength() +
> 1, or something like that
>
> On Wednesday, October 8, 2003, at 11:17 PM, Robert Wilhelm wrote:
> > I have made some test replacing eplacing UT_StringPtrMap with
> > UT_UTF8Hash in pp_TableAttrProp.cpp.
> > It is not finished, I just converted m_pAttributes but not
> > yet m_pProperties. Also it might leak memory. It needs
> > some cleanup. You name it.
> >
> > But at least it compiles and I was able to load the RTF spec.
> > As I won´t be able to hack during the next days,
> > I have attached my current patch so it cannot get lost.
> > ML archives are a such a nice backup medium :-)
>
> Regards, Frank
>
> 9. Has the work, for you, a metaphysical dimension? Yes ( ) No ( )
> 10. What is it (twenty-five words or less)?
>
> - Snow White, Donald
> Barthelme
>
This archive was generated by hypermail 2.1.4 : Thu Oct 09 2003 - 18:38:03 EDT