rename ENABLE_DEBUG to MRB_DEFINE_HOOKS #3014

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@cremno
Contributor

cremno commented Nov 16, 2015

Here are some reasons why it should be renamed:

  • ENABLE_DEBUG isn't prefixed (possible name clash with other libs)
  • the enable_debug build config method is totally unrelated to this
  • the new name MRB_DEFINE_HOOKS is more precise

ENABLE_DEBUG and DISABLE_DEBUG will still work for backward compatibility but mruby users should update their code. E.g. to #if defined MRB_DEFINE_HOOKS || defined ENABLE_DEBUG.

rename ENABLE_DEBUG to MRB_DEFINE_HOOKS
Here are some reasons why it should be renamed:

- ENABLE_DEBUG isn't prefixed (possible name clash with other libs)
- the enable_debug build config method is totally unrelated to this
- the new name MRB_DEFINE_HOOKS is more precise

ENABLE_DEBUG and DISABLE_DEBUG will still work for backward
compatibility but mruby users should update their code.
E.g. to `#if defined MRB_DEFINE_HOOKS || defined ENABLE_DEBUG`.
@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Nov 16, 2015

Member

It's OK to rename it, but I don't think MRB_DEFINE_HOOK is not precise. It sounds like the hook itself, besides that, it doesn't specify which hook we are going to add. How about 'MRB_ENABLE_CODE_FETCH_HOOK`. The only concern is it's too long.

Member

matz commented Nov 16, 2015

It's OK to rename it, but I don't think MRB_DEFINE_HOOK is not precise. It sounds like the hook itself, besides that, it doesn't specify which hook we are going to add. How about 'MRB_ENABLE_CODE_FETCH_HOOK`. The only concern is it's too long.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Nov 16, 2015

Member

And what do you think about another ENABLE_ macro, ENABLE_STDIO?

Member

matz commented Nov 16, 2015

And what do you think about another ENABLE_ macro, ENABLE_STDIO?

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Nov 16, 2015

Member

In any case, I think it should be prefixed with MRB_ to avoid conflicts with other libraries.

Member

zzak commented Nov 16, 2015

In any case, I think it should be prefixed with MRB_ to avoid conflicts with other libraries.

@cremno

This comment has been minimized.

Show comment
Hide comment
@cremno

cremno Nov 16, 2015

Contributor

What about MRB_DEBUG_HOOKS? My only concern is that people might think it's related to MRB_DEBUG.

What do you think about MRB_NO_STDIO or the opposite MRB_USE_STDIO (defined by default)?

Contributor

cremno commented Nov 16, 2015

What about MRB_DEBUG_HOOKS? My only concern is that people might think it's related to MRB_DEBUG.

What do you think about MRB_NO_STDIO or the opposite MRB_USE_STDIO (defined by default)?

@cremno

This comment has been minimized.

Show comment
Hide comment
@cremno

cremno Nov 16, 2015

Contributor

Or MRB_STATE_DEFINE_HOOKS or just MRB_STATE_HOOKS?

Contributor

cremno commented Nov 16, 2015

Or MRB_STATE_DEFINE_HOOKS or just MRB_STATE_HOOKS?

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Nov 16, 2015

Member

I picked MRB_ENABLE_DEBUG_HOOK for consistency with (MRB_)DISABLE_STDIO.

Member

matz commented Nov 16, 2015

I picked MRB_ENABLE_DEBUG_HOOK for consistency with (MRB_)DISABLE_STDIO.

@matz matz closed this in 4440566 Nov 16, 2015

@cremno

This comment has been minimized.

Show comment
Hide comment
@cremno

cremno Nov 16, 2015

Contributor

Okay. By the way, I've proposed MRB_NO_STDIO because mruby already has MRB_NO_INIT_ARRAY_START.

Contributor

cremno commented Nov 16, 2015

Okay. By the way, I've proposed MRB_NO_STDIO because mruby already has MRB_NO_INIT_ARRAY_START.

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Nov 17, 2015

Member

@matz @cremno The only problem I have with the naming MRB_ENABLE_DEBUG_HOOK is it's similar to MRB_DEBUG. This is probably MRB_DEBUGs fault for being too generic though.

Thoughts?

Member

zzak commented Nov 17, 2015

@matz @cremno The only problem I have with the naming MRB_ENABLE_DEBUG_HOOK is it's similar to MRB_DEBUG. This is probably MRB_DEBUGs fault for being too generic though.

Thoughts?

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Nov 17, 2015

Member

@zzak probably I will rename MRB_DEBUG later, but not this time.

Member

matz commented Nov 17, 2015

@zzak probably I will rename MRB_DEBUG later, but not this time.

@tsahara tsahara referenced this pull request in iij/mruby Dec 8, 2015

Closed

make December 2015 Stable Release #155

68 of 69 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment