Fix arm-none-eabi build (fixes #451) #496

ebraminio · 2017-06-13T16:26:44Z

This Is a part of porting harfbuz to vitasdk, I guess "src/hb-ot-layout.cc" change part, specially, needs more careful reviewing.

ebraminio · 2017-06-13T17:55:09Z

This makes vitasdk package builder CI work also: vitasdk/packages#28

ebraminio · 2017-06-18T14:06:25Z

src/hb-ot-layout.cc

@@ -279,10 +279,9 @@ hb_ot_layout_get_ligature_carets (hb_font_t      *font,
 				  hb_codepoint_t  glyph,
 				  unsigned int    start_offset,
 				  unsigned int   *caret_count /* IN/OUT */,
-				  int            *caret_array /* OUT */)
+				  hb_position_t  *caret_array /* OUT */)


Now it matches with its actual signature on src/hb-ot-layout.h

ebraminio · 2017-06-18T14:10:02Z

src/hb-ot-font.cc

@@ -44,7 +44,7 @@
 struct hb_ot_face_metrics_accelerator_t
 {
  unsigned int num_metrics;
-  unsigned int num_advances;
+  uint32_t num_advances;


This would be safe I'd guess AFAIK but have a careful look please

needed as https://github.com/behdad/harfbuzz/blob/79e8e27ffd3da29ca27d3aebd2ef425bf1cb7f9d/src/hb-ot-font.cc#L141 glyph is uint32_t so this should be also (or I should cast there instead)

Meh. This case, cast would have been nice. But doesn't matter. Maybe change num_metrics to uint32_t as well?

np, went for cast

ebraminio · 2017-06-18T14:11:23Z

src/hb-common.cc

@@ -671,6 +671,30 @@ parse_uint (const char **pp, const char *end, unsigned int *pv)
 }

 static bool
+parse_uint32 (const char **pp, const char *end, uint32_t *pv)


Duplication of parse_uint, no template please as it opens another can of worms on different platforms I'd think :)

ebraminio · 2017-06-18T14:20:22Z

src/hb-ot-shape-complex-arabic-table.hh

@@ -139,28 +139,28 @@ joining_type (hb_codepoint_t u)
  switch (u >> 12)
  {
    case 0x0u:
-      if (hb_in_range (u, 0x0600u, 0x08E2u)) return joining_table[u - 0x0600u + joining_offset_0x0600u];
+      if (hb_in_range<hb_codepoint_t> (u, 0x0600u, 0x08E2u)) return joining_table[u - 0x0600u + joining_offset_0x0600u];


If I change 0x0600u to 0x0600uL it works on arm-none-eabi but not on my default build system, so best way here would be disambiguating it I'd guess.

Another option would be to define a macro U:

#define U(v) ((hb_codepoint_t) (v)) and then use `U(0x0600)` etc. Or even further, dropping the 0x, so we can say eg. `u(0600)`. But we can do that in the future.

I was doing this but found out U already has a conflicting use.

Not in favor of u() personally as it will obfuscating things more on my view, lets defer it if is possible.

Template disambiguating had before use on the project so they are not that unusual I'd guess

behdad · 2017-06-19T05:22:15Z

src/hb-common.cc

-  bool had_value = parse_uint (pp, end, &feature->value) ||
-                   parse_bool (pp, end, &feature->value);
+  bool had_value = parse_uint (pp, end, (unsigned int*) &feature->value) ||
+                   parse_bool (pp, end, (unsigned int*) &feature->value);


Another option would be to define a local variable of appropriate type, and assign that to feature->value later. Then you wouldn't need duplicating parse_uint and parse_bool etc.

On that case I should interfere more with the logic as putting result on the OUT parameter is not happening always so I should check one first then the other ... let defer that.

behdad · 2017-06-19T05:22:52Z

As we discussed yesterday, the (unsigned int*) &feature->value is not correct. Feel free to merge after addressing that one. I left a few other comments as well, feel free to address.

ebraminio · 2017-06-19T10:03:18Z

src/hb-ot-font.cc

@@ -138,7 +138,7 @@ struct hb_ot_face_metrics_accelerator_t
 	return this->default_advance;
    }

-    return this->table->longMetric[MIN (glyph, this->num_advances - 1)].advance
+    return this->table->longMetric[MIN (glyph, (uint32_t) this->num_advances - 1)].advance


As requested instead interfering with "num_advances" type

ebraminio · 2017-06-19T10:20:27Z

CMakeLists.txt

@@ -128,7 +128,7 @@ set(HB_VERSION_MICRO ${CMAKE_MATCH_4})

 ## Define ragel tasks
 if (NOT IN_HB_DIST)
-  find_program(RAGEL "ragel")
+  find_program(RAGEL "ragel" CMAKE_FIND_ROOT_PATH_BOTH)


So it can find ragel on vitasdk environment.

ebraminio · 2017-06-19T10:20:41Z

CMakeLists.txt

@@ -71,7 +71,7 @@ endif ()

 ## Detect if we are running inside a distribution or regular repository folder
 set(IN_HB_DIST FALSE)
-if (EXISTS "${PROJECT_SOURCE_DIR}/src/hb-version.h")
+if (EXISTS "${PROJECT_SOURCE_DIR}/ChangeLog")


Better is-in-distribution check

behdad · 2017-06-21T10:58:55Z

Thanks. Please merge as you see fit (and if the bots are green).

ebraminio requested a review from behdad June 13, 2017 16:26

This was referenced Jun 13, 2017

Add harfbuzz 1.4.6 vitasdk/packages#28

Merged

PDF rendering library pathway27/bookr-mod-vita#2

Closed

Fix arm-none-eabi build (fixes harfbuzz#451)

9fc433b

ebraminio force-pushed the armfix branch from c04d729 to 9fc433b Compare June 13, 2017 17:16

ebraminio mentioned this pull request Jun 13, 2017

Fails to build with arm-none-eabi Newlib toolchain #451

Closed

ebraminio commented Jun 18, 2017

View reviewed changes

ebraminio force-pushed the armfix branch from 2f0b872 to f2d0e8c Compare June 18, 2017 14:07

Address offline reviews

978770d

ebraminio force-pushed the armfix branch from f2d0e8c to 978770d Compare June 18, 2017 14:07

ebraminio commented Jun 18, 2017

View reviewed changes

behdad reviewed Jun 19, 2017

View reviewed changes

Address review and fix ragel finding while on vitasdk environment

c3e6d01

ebraminio commented Jun 19, 2017

View reviewed changes

ebraminio merged commit 3b0e47c into harfbuzz:master Jun 19, 2017

ebraminio deleted the armfix branch June 19, 2017 10:17

ebraminio commented Jun 19, 2017

View reviewed changes

ebraminio mentioned this pull request Oct 31, 2017

Add more CI environments #589

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix arm-none-eabi build (fixes #451) #496

Fix arm-none-eabi build (fixes #451) #496

ebraminio commented Jun 13, 2017

ebraminio commented Jun 13, 2017

ebraminio Jun 18, 2017

ebraminio Jun 18, 2017

ebraminio Jun 18, 2017

behdad Jun 19, 2017

ebraminio Jun 19, 2017

ebraminio Jun 18, 2017

ebraminio Jun 18, 2017 •

edited

behdad Jun 19, 2017

ebraminio Jun 19, 2017 •

edited

behdad Jun 19, 2017

ebraminio Jun 19, 2017

behdad commented Jun 19, 2017

ebraminio Jun 19, 2017 •

edited

ebraminio Jun 19, 2017

ebraminio Jun 19, 2017

behdad commented Jun 21, 2017

Fix arm-none-eabi build (fixes #451) #496

Fix arm-none-eabi build (fixes #451) #496

Conversation

ebraminio commented Jun 13, 2017

ebraminio commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebraminio Jun 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebraminio Jun 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad commented Jun 19, 2017

ebraminio Jun 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behdad commented Jun 21, 2017

ebraminio Jun 18, 2017 •

edited

ebraminio Jun 19, 2017 •

edited

ebraminio Jun 19, 2017 •

edited