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

[RFC] Remove dead #ifdefed code #814

Merged
merged 14 commits into from Aug 7, 2014

Conversation

Projects
None yet
4 participants
@Hinidu
Contributor

Hinidu commented Jun 5, 2014

I made a simple analyzer to find symbols which are tested in #ifdefs but are not defined explicitly.
Now I want to go through the resulting list step by step and remove dead code related to legacy systems and other stuff which we will not use in the future.
Inspired by #808

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Jun 5, 2014

Member

Probably simple for everyone who reads Haskell fluently ;). Just kidding though, this could be great!

Member

aktau commented Jun 5, 2014

Probably simple for everyone who reads Haskell fluently ;). Just kidding though, this could be great!

@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Jun 5, 2014

Contributor

Probably simple for everyone who reads Haskell fluently ;). Just kidding though, this could be great!

I mean simple algorithm, not simple code :-) Btw I tried to do my best to simplify code if someone would be interested in it.
PS: Initially I tried to use Lua's Lpeg but I am too weak in it for a while being.

Contributor

Hinidu commented Jun 5, 2014

Probably simple for everyone who reads Haskell fluently ;). Just kidding though, this could be great!

I mean simple algorithm, not simple code :-) Btw I tried to do my best to simplify code if someone would be interested in it.
PS: Initially I tried to use Lua's Lpeg but I am too weak in it for a while being.

@equalsraf

This comment has been minimized.

Show comment
Hide comment
@equalsraf

equalsraf Jun 5, 2014

Contributor

Nicely done. I've been doing some cleaning and came across some of those already:

  • USE_ICONV is not being used right now but seems to be perfectly functional - #813 - I have no idea about DYNAMIC_ICONV
  • Just bumped into S_ISLNK, S_* in #810
  • USE_TMPNAM is related to #812
Contributor

equalsraf commented Jun 5, 2014

Nicely done. I've been doing some cleaning and came across some of those already:

  • USE_ICONV is not being used right now but seems to be perfectly functional - #813 - I have no idea about DYNAMIC_ICONV
  • Just bumped into S_ISLNK, S_* in #810
  • USE_TMPNAM is related to #812
@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Jun 6, 2014

Contributor

@equalsraf thank you for information. I will take this into account.

Contributor

Hinidu commented Jun 6, 2014

@equalsraf thank you for information. I will take this into account.

@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Jun 6, 2014

Contributor

I need advise on my last commit "Remove FEAT_OSFILETYPE". That feature is disabled in Vim, but was not entirely removed to use it in the future on operating systems which support storing a file type with the file (see more details in commit message). I am not sure that some of Neovim target platforms have such functionality. Does anyone have some insights?

Contributor

Hinidu commented Jun 6, 2014

I need advise on my last commit "Remove FEAT_OSFILETYPE". That feature is disabled in Vim, but was not entirely removed to use it in the future on operating systems which support storing a file type with the file (see more details in commit message). I am not sure that some of Neovim target platforms have such functionality. Does anyone have some insights?

@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Jun 7, 2014

Contributor

I removed all defines which look to me obviously unneeded. I have doubts only about FEAT_OSFILETYPE(I mentioned it in the previous comment). And I left __BEOS__ because I don't know what to do with it in term.c, as well as __OPENNT, __TANDEM and __QNXNTO__ because I'm not sure whether we support these platforms or not.
If nobody has suggestions then probably I will not do anything else in this PR.
Marked as RFC.

Contributor

Hinidu commented Jun 7, 2014

I removed all defines which look to me obviously unneeded. I have doubts only about FEAT_OSFILETYPE(I mentioned it in the previous comment). And I left __BEOS__ because I don't know what to do with it in term.c, as well as __OPENNT, __TANDEM and __QNXNTO__ because I'm not sure whether we support these platforms or not.
If nobody has suggestions then probably I will not do anything else in this PR.
Marked as RFC.

@Hinidu Hinidu changed the title from [WIP] Remove dead #ifdefed code to [RFC] Remove dead #ifdefed code Jun 7, 2014

@justinmk justinmk added the refactor label Jul 1, 2014

Hinidu added some commits Jun 5, 2014

Remove BeOS DR8 specific hack
It is already partially removed from screen.c
Show +cursorshape in :version
All code which was inside #ifdef CURSOR_SHAPE is being used now, except
one in version.c (that occurence is fixed by this commit).
Remove FEAT_OSFILETYPE
Feature description from Vim documentation:

NOTE: this code is currently disabled, as the RISC OS implementation was
removed.  In the future this will use the 'filetype' option.

On operating systems which support storing a file type with the file, you can
specify that an autocommand should only be executed if the file is of a
certain type.

The actual type checking depends on which platform you are running Vim
on; see your system's documentation for details.

To use osfiletype checking in an autocommand you should put a list of types to
match in angle brackets in place of a pattern, like this: >

	:au BufRead *.html,<&faf;HTML>  runtime! syntax/html.vim

This will match:

- Any file whose name ends in ".html"
- Any file whose type is "&faf" or "HTML", where the meaning of these types
  depends on which version of Vim you are using.
  Unknown types are considered NOT to match.
Remove FEAT_TAG_ANYWHITE
This feature allow to use any white space characters instead of one
<TAB> in tag files. It is disabled in vanilla Vim's default build
configuration. Exuberant ctags use format with exactly one TAB.
Remove HAVE_TOTAL_MEM
libuv provide uv_get_total_mem_kib. So HAVE_TOTAL_MEM should always be
true.
Before that commit in neovim maxmem=5120 and maxmemtot=10240. Now
both equal to half of system memory.
Remove HAVE_LIBC_H
It was used in Vim for NeXT OS.
@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Aug 2, 2014

Contributor

Rebased.
Is it worth to split this PR to many by topics like BeOS, EBCDIC, etc? Since now they should involve some changes in the documentation, so PR will be even bigger.

Contributor

Hinidu commented Aug 2, 2014

Rebased.
Is it worth to split this PR to many by topics like BeOS, EBCDIC, etc? Since now they should involve some changes in the documentation, so PR will be even bigger.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 2, 2014

Member

@Hinidu if you could update the docs that would be great, and splitting this into say 3 PRs would make it much easier to digest. I was wanting to get this PR through soon.

Member

justinmk commented Aug 2, 2014

@Hinidu if you could update the docs that would be great, and splitting this into say 3 PRs would make it much easier to digest. I was wanting to get this PR through soon.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Aug 2, 2014

Member

I don't see the need to split this into multiple PRs, it's perfectly manageable and self-contained as it is. In my opinion.

Member

aktau commented Aug 2, 2014

I don't see the need to split this into multiple PRs, it's perfectly manageable and self-contained as it is. In my opinion.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 2, 2014

Member

Ok. But it seems like no one has had time to review it so I inferred that it was due to the size.

Member

justinmk commented Aug 2, 2014

Ok. But it seems like no one has had time to review it so I inferred that it was due to the size.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Aug 2, 2014

Do we still even use the MAC define? Perhaps something to keep in mind for another cleanup.

aktau commented on src/nvim/option.c in 8991609 Aug 2, 2014

Do we still even use the MAC define? Perhaps something to keep in mind for another cleanup.

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Aug 4, 2014

Owner

My script also marked this macros as "never defined". But cleaning it up is more complicated because we should analyze which parts of Mac-specific code was used for modern OS X and which not.

Owner

Hinidu replied Aug 4, 2014

My script also marked this macros as "never defined". But cleaning it up is more complicated because we should analyze which parts of Mac-specific code was used for modern OS X and which not.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Aug 2, 2014

Member

Ok. But it seems like no one has had time to review it so I inferred that it was due to the size.

I guess the amount of commits does "scare" a little bit. Anyway, to counteract that I gave it a quick look and it looks quite good. I just had one thought:

Don't have a lot of knowledge of EBCDIC, but after looking at that wiki page, I can see that it's really ancient (IBM mainframes?) Good riddance. That said, sometimes some people seem to want it?! http://www.sublimetext.com/forum/viewtopic.php?f=4&p=42210

I'd rather see it go, people that want compatibility with such a thing could perhaps use a plugin or vanilla.

LGTM.

Member

aktau commented Aug 2, 2014

Ok. But it seems like no one has had time to review it so I inferred that it was due to the size.

I guess the amount of commits does "scare" a little bit. Anyway, to counteract that I gave it a quick look and it looks quite good. I just had one thought:

Don't have a lot of knowledge of EBCDIC, but after looking at that wiki page, I can see that it's really ancient (IBM mainframes?) Good riddance. That said, sometimes some people seem to want it?! http://www.sublimetext.com/forum/viewtopic.php?f=4&p=42210

I'd rather see it go, people that want compatibility with such a thing could perhaps use a plugin or vanilla.

LGTM.

@Hinidu

This comment has been minimized.

Show comment
Hide comment
@Hinidu

Hinidu Aug 4, 2014

Contributor

@Hinidu if you could update the docs that would be great

@justinmk I'll try to do this as separate commits in the next few days.

splitting this into say 3 PRs would make it much easier to digest

Since @aktau already reviewed this PR (thank you @aktau!) is this still necessary?

Contributor

Hinidu commented Aug 4, 2014

@Hinidu if you could update the docs that would be great

@justinmk I'll try to do this as separate commits in the next few days.

splitting this into say 3 PRs would make it much easier to digest

Since @aktau already reviewed this PR (thank you @aktau!) is this still necessary?

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 4, 2014

Member

No, don't worry about splitting it.

Member

justinmk commented Aug 4, 2014

No, don't worry about splitting it.

@justinmk justinmk merged commit 8b72ae7 into neovim:master Aug 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

justinmk added a commit that referenced this pull request Aug 7, 2014

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Aug 7, 2014

Member

Thank you @Hinidu

Member

justinmk commented Aug 7, 2014

Thank you @Hinidu

@Hinidu Hinidu deleted the Hinidu:remove-unused-def-symbols branch Aug 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment