Re: serhat - r31414 - in abiword/branches/gsoc2012docx/plugins/openxml: common/xp exp/xp imp/xp

On 24/06/12 09:24 AM, [email protected] wrote:
>
> 
> Modified: abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.cpp
> ===================================================================
> --- abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.cpp	2012-06-23 05:10:59 UTC (rev 31413)
> +++ abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.cpp	2012-06-24 16:24:11 UTC (rev 31414)
> @@ -24,6 +24,9 @@
>  // Class definition include
>  #include <ie_exp_OpenXML.h>
>  
> +// Abiword includes
> +#include <ut_std_string.h>
> +
>  /**
>   * Constructor
>   */
> @@ -2838,36 +2841,38 @@
>  	return UT_OK;
>  }
>  
> -UT_Error IE_Exp_OpenXML::startStyle(std::string style, std::string basedon, std::string followedby)
> +UT_Error IE_Exp_OpenXML::startStyle(std::string style, std::string basedon, std::string followedby, std::string type)
>  {
> -	UT_UTF8String sEscStyle = style.c_str();
> -	UT_UTF8String sEscBasedOn = basedon.c_str();
> -	UT_UTF8String sEscFollowedBy = followedby.c_str();
> +	std::string sEscStyle = UT_escapeXML(style.c_str());
> +	std::string sEscBasedOn = UT_escapeXML(basedon.c_str());
> +	std::string sEscFollowedBy = UT_escapeXML(followedby.c_str());
> +	std::string sEscType = UT_escapeXML(type.c_str());

UT_escapeXML takes a const std::string &, so you should be calling
c_str() on them to pass them.
(I wish there was a way to force this right now)

> 
> Modified: abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.h
> ===================================================================
> --- abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.h	2012-06-23 05:10:59 UTC (rev 31413)
> +++ abiword/branches/gsoc2012docx/plugins/openxml/exp/xp/ie_exp_OpenXML.h	2012-06-24 16:24:11 UTC (rev 31414)
> @@ -86,7 +86,7 @@
>  	UT_Error finishParagraphProperties(int target);
>  	UT_Error startCellProperties(int target);
>  	UT_Error finishCellProperties(int target);
> -	UT_Error startStyle(std::string name, std::string basedon, std::string followedby);
> +	UT_Error startStyle(std::string name, std::string basedon, std::string followedby, std::string type);


I don't want to open a can of worm, but this is where you should also
change the signature to pass a const std::string & instead of a
std::string (that will be copy). Much more efficient.


> Modified: abiword/branches/gsoc2012docx/plugins/openxml/imp/xp/OXMLi_ListenerState_Common.cpp
> ===================================================================
> --- abiword/branches/gsoc2012docx/plugins/openxml/imp/xp/OXMLi_ListenerState_Common.cpp	2012-06-23 05:10:59 UTC (rev 31413)
> +++ abiword/branches/gsoc2012docx/plugins/openxml/imp/xp/OXMLi_ListenerState_Common.cpp	2012-06-24 16:24:11 UTC (rev 31414)

[...]

> +		if(isOverline != std::string::npos && isUnderline == std::string::npos)
> +		{
> +			// if starts with "EQ \x \to", then overline property is used
> +			UT_return_if_fail( this->_error_if_fail( UT_OK == run->setProperty("text-decoration", "overline") ));
> +			rqst->stck->push(sharedElem);
> +		}
> +		else if(isUnderline != std::string::npos && isOverline == std::string::npos)
> +		{
> +			// if starts with "EQ \x \bo", then underline property is used
> +			UT_return_if_fail( this->_error_if_fail( UT_OK == run->setProperty("text-decoration", "underline") ));
> +			rqst->stck->push(sharedElem);
> +		}

I know it is done elsewhere, but calling this kind of mutation function
like setProperty inside UT_return_if_fail() is not a good idea as I am
quasi certain that UT_return_if_fail() is what is causing some of our
file corruption bugs by exiting early because it is being abused.

Do the action then check the error.


Hub