-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 unused const FEAT_GUI* #2609
[RFC] Remove unused const FEAT_GUI* #2609
Conversation
isatty(2) | ||
# endif | ||
) { | ||
if (isatty(2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably use os_isatty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You said I just have to replace every isatty
with os_isatty without any other check ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without any other check ?
What do you mean? I just mean replace the two instances of isatty
in this PR with os_isatty
, since they have the same semantics AFAICT:
/// Test whether a file descriptor refers to a terminal.
///
/// @param fd File descriptor.
/// @return `true` if file descriptor refers to a terminal.
bool os_isatty(int fd)
{
return uv_guess_handle(fd) == UV_TTY;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if that came across as overly blunt, I'm just trying to be clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "without any other check" I mean 'I don't now what really do os_isatty
instead of isatty
so I asked if I really can do this without any other check.
isatty replaced in commit a083983
Besides the nit, LGTM. |
While greping the source for FEAT_GUI, I found some more unused |
Thank you. I will do that in this PR as soon as possible |
I change this PR as WIP because I found lot of const no more used and I want to clean it up. |
I think it is good. Change from WIP to RFC. |
@@ -119,7 +119,6 @@ The following compile time preprocessor variables control the features: | |||
Debug Glyphs FEAT_SIGNS | |||
Debug Highlights FEAT_SIGNS | |||
Message Footer FEAT_FOOTER | |||
Balloon Evaluation FEAT_BEVAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a lot of references in the docs to this, but that should probably be dealt with elsewhere so I suppose it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, about these const : FEAT_SIGNS, FEAT_FOOTER and FEAT_BEVAL, they only appears in this doc.
I wasn't sure if I can delete all the 2. Vim Compile Options *debugger-compilation*
paragraph . Should I do this in a commit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if I can delete all the 2. Vim Compile Options debugger-compilation paragraph . Should I do this in a commit ?
Good idea, but do you mind If instead I do that in #2532? That PR is mostly about documentation while this one already has a pretty specific focus (cleaning up macros). I'll be sure to credit your in the commit message, but it's up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you do decide to do it here, make sure to remove section 3 as well: it's empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I let you do this. And don't credit me, I just revealed a probably dead doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And don't credit me, I just revealed a probably dead doc.
I just put spotted by @Hettomei
, that's all :-)
Besides the nits on the commit structure, LGTM. Good thing you mentioned those defines. |
69188a0
to
ff56e7a
Compare
I rebased on master and squashed/changed logs according to comments. I have done a |
Yes, that's fine. Could you take a lot at this @mhinz and/or @splinterofchaos? I'm not 100 percent sure this code won't be needed in the future at some point. |
I'll just add a waiting label so this isn't forgotten. |
if (menu->children == NULL) { | ||
/* found a menu item instead of a sub-menu */ | ||
if (*p == NUL) | ||
EMSG(_("E336: Menu path must lead to a sub-menu")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that message E336 is gone you can delete the E336 messages in src/nvim/po/*.po
. And perhaps also the tag *E336*
in runtime/doc/gui.txt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that they they shouldn't be deleted as they're used for generating fuzzy translations at compile time. Not completely sure though, cc @fwalch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to commit right now... I wait.
Also there is E338, and E340 (and I think many more) which is never found in source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, translations shouldn't be deleted from the PO files, even if they're unused. They might be useful for fuzzy translations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the insight.
I m newbie to C, never used po files and english is not my native language so : what concretly mean "might be useful for fuzzy translations" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fuzzy translations (as opposed to "manual" translations done by a human) are automatic translations where an existing translation is used for a similar, but different message. Obviously it won't be an exact translation, hence "fuzzy".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
This function is only used for windows 'mch_resolve_shortcut' can be found in vim source src/os_mswin.c -> mch_resolve_shortcut(char_u *fname) Spotted by @equalsraf, see PR neovim#2609
These variables are no more needed so I removed it.
And also it simplify logic for mch_errmsg and mch_msg because we can group UNIX code.