[Poppler-bugs] [Bug 64815] [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=64815

--- Comment #54 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 87701
  --> https://bugs.freedesktop.org/attachment.cgi?id=87701
[PATCH v9 01/12] Tagged-PDF: Parsing of StructElem standard types and
attributes

Review of attachment 87701:
-----------------------------------------------------------------

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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20131017/17ed1b9a/attachment.html>