Re: frt - r26239 - in abiword/branches/gsoc2009oox/plugins/openxml: common/xp exp/xp imp/xp

On 05/09/2009 11:12 PM, cvs@abisource.com wrote:
> Author: frt

A few comments.

> -OXML_Element_Cell::OXML_Element_Cell(std::string id, OXML_Element_Table* tbl, UT_sint32 left, UT_sint32 right, UT_sint32 top, UT_sint32 bottom) :
> +//External includes
> +#include<sstream>
> +
> +OXML_Element_Cell::OXML_Element_Cell(std::string id, OXML_Element_Table* tbl, OXML_Element_Row* rw,
> + UT_sint32 left, UT_sint32 right, UT_sint32 top, UT_sint32 bottom) :

1/ cosmetic but usually system headers (like sstream) are included
first. How see below for the use of sstream.
2/ pass id as a const std::string &. It is always cheaper.

> + //add props:bot-attach, left-attach, right-attach, top-attach
> + std::stringstream out;
> + out<< m_iTop;
> + std::string sTop = out.str();
> + out.str("");
> + out<< m_iBottom;
> + std::string sBottom = out.str();
> + out.str("");
> + out<< m_iLeft;
> + std::string sLeft = out.str();
> + out.str("");
> + out<< m_iRight;
> + std::string sRight = out.str();

Now I have some concern about the use of std::stringstream. Nowhere else
we do than in AbiWord.

> +
> + ret = setProperty("top-attach", sTop.c_str());

and then here setProperty() should probably be overloaded to take a cons
std::string as the second argument.
But then maybe addressing the above comment will make this one obsolete.

Hub
Received on Mon May 11 08:59:30 2009