Comment 11 for bug 1318657

Revision history for this message
Johan Engelen (johanengelen) wrote :

Comments about the patch in #10:

- Please reformat the code.
      if(defid.find(DuplicateDefString) != Glib::ustring::npos)break;
  is very hard to read.

- Could you change the "if (it->type == REF_HREF)" list to a switch statement?

- This section
+ SPObject *trg = source->getObjectByRepr(laterDef);
+ Glib::ustring newid = trg->getId();
+ if(newid.find(DuplicateDefString) != Glib::ustring::npos)continue; // this one already handled
+ if (trg && SP_IS_GRADIENT(trg) && src != trg) {
  crashes if trg == nullptr.
  Also, please don't rely on operator precedence and put parens around src!=trg

- About
+ if (gr->isEquivalent(SP_GRADIENT(trg)) &&
+ gr->isAligned(SP_GRADIENT(trg))) {
   SP_GRADIENT(trg) is potentially expensive, so don't duplicate it unnecessarily.

- About
+ gboolean duplicate = false;
  Use C++ bool instead.