Is char* or uintptr_t easier to read?

Is char* or uintptr_t easier to read?

Using a char* instead of uintptr_t allows us to use fewer
reinterpret_casts which may make the code easier to read.

BUG=648512
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2356703002

Committed: https://skia.googlesource.com/skia/+/921bc678a7b81de31b4e672326ed2f37ffb66b10

[cblume, 2016-09-20 06:17:06]

Description was changed from

==========
Is char* or uintptr_t easier to read?

Using a char* instead of uintptr_t allows us to use fewer
reinterpret_casts which may make the code easier to read.

BUG=648512
==========

to

==========
Is char* or uintptr_t easier to read?

Using a char* instead of uintptr_t allows us to use fewer
reinterpret_casts which may make the code easier to read.

BUG=648512
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2356703002
==========

[cblume, 2016-09-20 06:19:16]

cblume@chromium.org changed reviewers:
+ bsalomon@google.com, ericrk@chromium.org

[cblume, 2016-09-20 06:19:16]

PTAL

This is only to see if we prefer this style of code for readability.

Using char* may allow us to use fewer reinterpret_casts, which might make the
code cleaner.

However, when we need to SkAlign8() we have to cast to uintrptr_t and back,
which is harder to read.

[Brian Osman, 2016-09-20 12:58:13]

On 2016/09/20 06:19:16, cblume wrote:
> PTAL
> 
> This is only to see if we prefer this style of code for readability.
> 
> Using char* may allow us to use fewer reinterpret_casts, which might make the
> code cleaner.
> 
> However, when we need to SkAlign8() we have to cast to uintrptr_t and back,
> which is harder to read.

Chiming in from the sideline - I usually prefer char* in these scenarios,
particularly because the alignment thing can be fixed by defining versions
of our alignment utilities that work on pointers.

[bsalomon, 2016-09-20 13:08:02]

I'd support adding SkAlign* variants that work on char*.

[reed1, 2016-09-20 13:40:08]

On 2016/09/20 13:08:02, bsalomon wrote:
> I'd support adding SkAlign* variants that work on char*.

+1

[ericrk, 2016-09-20 14:43:08]

On 2016/09/20 13:40:08, reed1 wrote:
> On 2016/09/20 13:08:02, bsalomon wrote:
> > I'd support adding SkAlign* variants that work on char*.
> 
> +1

+1 for adding these and using char*

[cblume, 2016-09-20 18:06:11]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 18:06:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2016-09-20 18:09:28]

Okay, sounds pretty unanimous that we all prefer char*. Thank you for that
recommendation, Eric! :D

Should we go ahead and land this patch?

Then I can make a separate patch with SkAlign* and loads of tests for it.

[commit-bot: I haz the power, 2016-09-20 18:43:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 18:43:45]

Dry run: This issue passed the CQ dry run.

[cblume, 2016-09-21 01:17:10]

On 2016/09/20 18:09:28, cblume wrote:
> Okay, sounds pretty unanimous that we all prefer char*. Thank you for that
> recommendation, Eric! :D
> 
> Should we go ahead and land this patch?
> 
> Then I can make a separate patch with SkAlign* and loads of tests for it.


Bug for SkAlign* changes:
https://bugs.chromium.org/p/chromium/issues/detail?id=648818

[bsalomon, 2016-09-21 13:10:28]

lgtm

[cblume, 2016-09-21 18:52:01]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:52:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 19:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 19:04:39]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[cblume, 2016-09-22 04:27:40]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2016-09-22 04:27:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 12:25:28]

Description was changed from

==========
Is char* or uintptr_t easier to read?

Using a char* instead of uintptr_t allows us to use fewer
reinterpret_casts which may make the code easier to read.

BUG=648512
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2356703002
==========

to

==========
Is char* or uintptr_t easier to read?

Using a char* instead of uintptr_t allows us to use fewer
reinterpret_casts which may make the code easier to read.

BUG=648512
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2356703002

Committed:
https://skia.googlesource.com/skia/+/921bc678a7b81de31b4e672326ed2f37ffb66b10
==========

[commit-bot: I haz the power, 2016-09-22 12:25:29]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/921bc678a7b81de31b4e672326ed2f37ffb66b10