Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RDY] Move arabic.h #defines from arabic.h to arabic.c and remove include of arabic.c #390

Closed
wants to merge 4 commits into from

Conversation

felipecrv
Copy link
Contributor

 - Turn ARABIC_CHAR into the arabic_char function
 - Move arabic_shape() decl from main.h to arabic.h
 - Move arabic_combine() and arabic_maycombine() from mbyte.c to
   arabic.c as these functions use the #defines I moved.
 - Remove the unnecessary include of arabic.h in globals.h
 - Remove include of arabic.c (sic) in main.c (change CMakeLists.txt to compile
   arabic.c normally)

Test plan: open a Arabic-Lorem-Ipsum.txt file :)

 - Move arabic_shape() decl from main.h to arabic.h
 - Move arabic_combine() and arabic_maycombine() from mbyte.c to
   arabic.c as these functions use the #defines I moved.
 - Remove the unnecessary include of arabic.h in globals.h
 - Remove include of arabic.c (sic) in main.c (change CMakeLists.txt to compile
   arabic.c normally)

// Range of Arabic characters that might be shaped.
#define ARABIC_CHAR(c) ((c) >= a_HAMZA && (c) <= a_MINI_ALEF)
int arabic_char(int c);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider putting arabic_char back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you mean? ARABIC_CHAR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily in form of a macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahkoh I moved it into arabic.c to be able to move all #defines there as well. Moving it back here would require me to move a_HAMZA, a_MINI_ALEF, and friends back.

The linker is able to inline external functions as well If you're worried about it [1].

[1] http://stackoverflow.com/questions/5987020/can-the-linker-inline-functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only have to define a_HAMZA and a_MINI_ALEF. And lto takes a lot of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahkoh if a_HAMZA and a_MINI_ALEF are to be moved, the whole alphabet should be moved.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Whether c belongs to the range of Arabic characters that might be shaped.
 static inline int arabic_char(int c)
 {
     //    c >= a_HAMZA && c <= a_MINI_ALEF
     return c >= 0xfe80 && c <= 0x0670;
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did it. Notice 0xfe80 is not a_HAMZA.

@felipecrv felipecrv changed the title [RFC] Move arabic.h #defines from arabic.h to arabic.c and remove include of arabic.c [RDY] Move arabic.h #defines from arabic.h to arabic.c and remove include of arabic.c Mar 24, 2014
@tarruda
Copy link
Member

tarruda commented Mar 24, 2014

Merged, ty

@tarruda tarruda closed this Mar 24, 2014
@felipecrv felipecrv deleted the arabic_defs branch April 7, 2014 14:05
dwb pushed a commit to dwb/neovim that referenced this pull request Feb 21, 2017
 - remove necessary checks and add %W to proselint.
 - add abort to neomake#makers#ft#tex#proselint()
 - move proselint maker to neomake#makers#ft#text#proselint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants