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

Add support for macros that take parameters #231

Closed
wants to merge 5 commits into from

Conversation

wheals
Copy link
Contributor

@wheals wheals commented Jul 21, 2015

Vezzra recently brought this up, and it had been something I wanted to take a crack at anyway.

I'm not really fond of the @1@ syntax, but it does the job of avoiding conflicts with FOCS syntax itself, which %1% would not.

The syntax is [[MACRO(a,b,...)]], and each one replaces the @1@, @2@, etc.
in the macro being replaced.
@@ -234,9 +235,9 @@ namespace parse {
const sregex MACRO_KEY = +_w; // word character (alnum | _), one or more times, greedy
const sregex MACRO_TEXT = -*_; // any character, zero or more times, not greedy
const sregex MACRO_DEFINITION = (s1 = MACRO_KEY) >> _n >> "'''" >> (s2 = MACRO_TEXT) >> "'''" >> _n;
const sregex MACRO_INSERTION = "[[" >> *space >> (s1 = MACRO_KEY) >> *space >> "]]";
const sregex MACRO_INSERTION = "[[" >> *space >> (s1 = MACRO_KEY) >> *space >> !("(" >> (s2 = +~_n) >> ")") >> "]]";
Copy link
Member

Choose a reason for hiding this comment

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

I think this MACRO_INSERTION definition could use some improvement. For our most typical use case, where the macro is used by itself on a line, as with the Xeno example, this looks fine to me. But if you were using it in the middle of some other text, it seems it could be at the the least horribly inefficient, but I think unless I'm mistaken, actually problematic. Consider the basic text

Ok this is some entry with a first macro useage [[ANY_MACRO(param1)]].  Then on the same line we also have [[ANY_MACRO(param2)]] and maybe a little more text.

When it hits that first ANY_MACRO, isn't s2 going to be all of

param1)]].  Then on the same line we also have [[ANY_MACRO(param2

?

It seems to me that rather than simply "+~_n" that you'd also want to exclude ")" in addition to the newlines-- it's not like anyone really needs to use a parenthesis as part of their macro substitution key.

@Dilvish-fo
Copy link
Member

In addition to my comment above, I do also want to make sure you get a "Bravo!" here-- it looks to me like this will be very helpful.

@wheals
Copy link
Contributor Author

wheals commented Jul 21, 2015

Would making it non-greedy also fix that first issue? I can't really speak to the efficiency of either approach, though...

@Dilvish-fo
Copy link
Member

Well, the non-greedy fixes it, but I think is less efficient-- with each nongreedy character it will try to move forward to the next parsing component, which I believes involving pushing some state info onto some kind of stack, and then will have to backtrack, again and again until it gets to the parenthesis. I think just excluding both newlines and parentheses would be much more efficient.

@Vezzra Vezzra added the category:refactoring The Issue/PR describes or contains an improved implementation. label Jul 22, 2015
@Dilvish-fo
Copy link
Member

it's not like anyone really needs to use a parenthesis as part of their macro substitution key.

I said that after just reviewing the code, and was a little rushed, didn't think about it enough-- that flaw in that statement is that it wasn't actually referring to the macro substitution key, it was referring to the macro substitution value, and people very well could reasonably want a parenthesis in it, and they could also want to have a comma in it too.

However, thinking this through a bit more, trying to deal with more complicated values would get rather, er, complicated. Particularly if we wanted to try making these things nestable the way our normal macros are. If we did also provide for more complicated substitution values, it would be ideal if simpler values could still be handled with this simpler syntax. So, I think I will do a bit of testing with the functionality as it is now, and then this PR should probably be ok to merge.

Just for brainstorming what might hopefully be a followup extension to this:

Rather than using a single character for the separator and a single character for the terminator, perhaps we could use some rare/odd character combination for them both. To allow things to also stay simple for the many times when the more complex handling isn't necessary, the basic treatment would be like you already have it, values comma separated and terminated by a parenthesis, except that if the value starts with a special combo (perhaps '@#@') then it goes on until that individual value is terminated with some special combo terminator (perhaps '@%@' ). The parsing of that could probably also keep track of nesting, I think.

@Dilvish-fo
Copy link
Member

Thinking it through more (and testing) it turns out that no change is needed to be able to have a substitution value include a right parenthesis-- it simply can't directly include it, but the value can itself contain a macro which in turn has the parenthesis.

I'm combining the parser changes into one commit, with the scripting change being a second commit.

This is very nice, thanks again, wheals.

@wheals wheals deleted the parametrised branch July 28, 2015 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:refactoring The Issue/PR describes or contains an improved implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants