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

Add "%elif" and "%else" directives for configuration file; allow nesting "%if" directives #1071

Closed
wants to merge 9 commits into
base: master
from

Conversation

3 participants
@townba

townba commented Sep 12, 2017

Add %elif and %else directives for configuration file. Allow nesting %if directives.

@ThomasAdam

This comment has been minimized.

Contributor

ThomasAdam commented Sep 12, 2017

I've not looked at this in much detail -- and although I appreciate queue.h has a lot of different list types, we've standardised on RB tree or TAILQs. I would suggest the use of a TAILQ instead of a SLIST.

I can't help but think that this augmentation of the simple %if syntax which was already in place rather overlaps with formats in a way which doesn't appreciably make a lot more sense to me.

@nicm

This comment has been minimized.

Contributor

nicm commented Sep 13, 2017

I think SLIST is OK so long as you remember not to use SLIST_REMOVE. But I would have used TAILQ and there isn't much benefit to using SLIST in code like this.

I think at this point the %if/%endif/etc code should be broken out of load_cfg into one or more helper functions; it might be better to do this first before adding %else.

@townba

This comment has been minimized.

townba commented Sep 13, 2017

I'll change it to TAILQ – I was considering it before anyway. I'll also break load_cfg up for readability.

@townba

This comment has been minimized.

townba commented Sep 21, 2017

Here's the latest. (I considered moving the cfg_dir_* functions into another file such as cfg-directive.c and maybe using "directive" rather than the abbreviation "dir", but I decided to see how this goes first.)

Attached is an example .tmux.conf file that exercises nested %if directives and error handling.

@nicm

This comment has been minimized.

Contributor

nicm commented Sep 22, 2017

OK looks good. Please try this, with the following changes:

  • I don't see why you need "met" and "meets" and "may_meet", it seems just "met" and "may_meet" (I renamed this to "skipped") would be enough. A condition is skipped if its parent condition was not met, and if it is skipped it and any else/elif/else also can't be met. Perhaps I am missing something though.
  • I don't see the need for a function pointer table here, so just use if/else.
  • Move more of the code out of load_cfg.
  • Rename some of the variables and functions and add some comments. Personal taste probably but anyway.
  • You should not need to be trimming spaces and handling comments so much: fparseln will remove comments, and load_cfg will trim spaces from start and end. So it is only necessary to trim trailing spaces before calling format_expand. And if we check for "%if " (with a space) etc then empty ones will be an error.
  • I don't think we need %error and if so we can do it separately.

Anyway please try this: x.diff.txt

@nicm

This comment has been minimized.

Contributor

nicm commented Sep 22, 2017

Oh also you can't do TAILQ_REMOVE after free()... :-)

@nicm nicm added the after-2.6 label Sep 27, 2017

@townba

This comment has been minimized.

townba commented Sep 29, 2017

Thank you very much for the review, @nicm. Notes:

  • You were right about only needing two variables to track conditions, though it needed more work to get nested %ifs working properly. Fixed.
  • Went with the if/else if approach to testing directives.
  • Moved code out of load_cfg à la your approach.
  • Used your names (and code comments) with just a few tweaks.
  • Removed most of the whitespace and comment handling I'd added.
    • Note: fparseln is being called without a comment delimiter (code), so it doesn't remove # comments. Instead, I'm simply ignoring any text on a line after %else and %endif.
  • I'll try to remember to remove %error. (I'm using it in my testing.)
  • Moved the free after the TAILQ_REMOVE.
  • Switched to use TAILQ_FOREACH_REVERSE_SAFE to dump out unterminated %ifs so they're now from earliest to latest.
@nicm

This comment has been minimized.

Contributor

nicm commented Sep 29, 2017

@nicm

This comment has been minimized.

Contributor

nicm commented Oct 1, 2017

Looks good, thanks. If you are going to skip spaces and still it is OK to make p char * instead of const char * and modify it, might be tidier. Anyway will have another look after 2.6 is released.

@townba

This comment has been minimized.

townba commented Oct 2, 2017

Text on a line after %else or %endif is now an error.

@nicm nicm removed the after-2.6 label Oct 5, 2017

@nicm

This comment has been minimized.

Contributor

nicm commented Oct 6, 2017

Applied to OpenBSD now, will be in GitHub later. Thanks!

@nicm nicm closed this Oct 6, 2017

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