<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present" href="https://bugs.freedesktop.org/show_bug.cgi?id=64815#c54">Comment # 54</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present" href="https://bugs.freedesktop.org/show_bug.cgi?id=64815">bug 64815</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=87701" name="attach_87701" title="[PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and attributes">attachment 87701</a> <a href="attachment.cgi?id=87701&action=edit" title="[PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and attributes">[details]</a> <a href='page.cgi?id=splinter.html&bug=64815&attachment=87701'>[review]</a> [PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and attributes Review of <a href="attachment.cgi?id=87701" name="attach_87701" title="[PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and attributes">attachment 87701</a> <a href="attachment.cgi?id=87701&action=edit" title="[PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and attributes">[details]</a> <a href='page.cgi?id=splinter.html&bug=64815&attachment=87701'>[review]</a>: ----------------------------------------------------------------- Looks good, I have just a few more comments ::: poppler/Makefile.am @@ +299,2 @@ > StructElement.cc \ > + StructTreeRoot.cc \ This is still unrelated to this patch. ::: poppler/StructElement.cc @@ +74,5 @@ > + || value->isName("End") > + || value->isName("Center"); > +} > + > +static GBool isNumber(Object *value); Simply move the implementation of isNumber before it's needed so that we don't need prototypes. You could move all isFoo methods for the basic types at the beginning. @@ +445,5 @@ > + attributeMapCommonList, > + NULL, > +}; > + > +static const AttributeMapEntry *attributeMapPrintField[] = { This is unused. @@ +493,5 @@ > + > + > +static GBool ownerHasMorePriority(Attribute::Owner a, Attribute::Owner b) > +{ > + unsigned a_index, b_index; aIndex, bIndex @@ +577,5 @@ > +//------------------------------------------------------------------------ > +// Helpers for the attribute and structure type tables > +//------------------------------------------------------------------------ > + > +static inline const AttributeMapEntry* AttributeMapEntry* -> AttributeMapEntry * @@ +594,5 @@ > + } > + return NULL; > +} > + > +static inline const AttributeMapEntry* AttributeMapEntry* -> AttributeMapEntry * @@ +635,5 @@ > + const OwnerMapEntry *entry = getOwnerMapEntry(owner); > + return entry ? entry->name : "UnknownOwner"; > +} > + > +Attribute::Owner nameToOwner(const char *name) This should be static @@ +695,5 @@ > + > + if (copyValue) > + valueA->copy(&value); > + else > + valueA->shallowCopy(&value); When does this happen? It seems that attributes are always created with copyValue = gFalse. @@ +712,5 @@ > + > + if (copyValue) > + valueA->copy(&value); > + else > + valueA->shallowCopy(&value); Ditto. @@ +716,5 @@ > + valueA->shallowCopy(&value); > + > + if (!checkType()) { > + type = Unknown; > + } You are not using braces for all other single statement ifs @@ +1322,5 @@ > + if (attribute->isOk() && (typeCheckOk = attribute->checkType(this))) { > + appendAttribute(attribute); > + } else { > + // It is not needed to free "value", the Attribute instance > + // owns the contents, so deleting "attribute" is enough. I think it's fine to always copy the value to make the code simpler. For heavy to copy objects we use reference counter.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>