We regularly run Coverity code scans on the OpenJDK sources and recently discovered two issues with HarfBuzz. While the discovered issues are not real errors, we think that fixing them my be nevertheless worthwile in order to increase the readability of the source code.

These fixes only make sense in the OpenJDK context if they are also accepted for inclusion in the upstream HarfBuzz repository and we will also propose a corresponding PullRequest there.

The first issue found by Coverity is the last of the following four lines from src/hb-ot-font.cc:

    if (!subtable) subtable = cmap->find_subtable (0, 2);
    if (!subtable) subtable = cmap->find_subtable (0, 1);
    if (!subtable) subtable = cmap->find_subtable (0, 0);
    if (!subtable)(subtable = cmap->find_subtable (3, 0)) && (symbol = true);

From the whole context it really took me some time to understand that 'symbol' should only be set to true if 'subtable' is set from 'cmap->find_subtable (3, 0)'. Coverity reports an "assignment instead of compare" which is a false positive, but we think the could would be much more readable if changed to look as follows:

    if (!subtable)
    {
      subtable = cmap->find_subtable (3, 0);
      if (subtable) symbol = true;
    }

The second issue is related to the following definition in src/hb-ot-layout-gpos-table.hh:

  ValueFormat valueFormat1; /* Defines the types of data in
                                        * ValueRecord1--for the first glyph
                                        * in the pair--may be zero (0) */
  ValueFormat valueFormat2; /* Defines the types of data in
                                        * ValueRecord2--for the second glyph
                                        * in the pair--may be zero (0) */

Throughout hb-ot-layout-gpos-table.hh, '&valueFormat1' is used as if it were an array of two ValueFormat objects. While extremely unlikely, a compiler could theoretically insert padding between 'valueFormat1' and 'valueFormat2' which would make the code incorrect. We would therefore propose to simply change the previous definiton into a real array:

  ValueFormat valueFormat[2]; /* [0] Defines the types of data in
                                        * ValueRecord1--for the first glyph
                                        * in the pair--may be zero (0) */
                                       /* [1] Defines the types of data in
                                        * ValueRecord2--for the second glyph
                                        * in the pair--may be zero (0) */

and change the code which uses 'valueFormat' accordingly.