Skip to content
This repository has been archived by the owner on Aug 6, 2023. It is now read-only.

Add new hash operator with precedence #37

Closed
wants to merge 18 commits into from
Closed

Add new hash operator with precedence #37

wants to merge 18 commits into from

Conversation

kymckay
Copy link
Contributor

@kymckay kymckay commented Jul 10, 2018

  • Ran the build database script to add the new 1.82 commands to the database.py file.
  • Prepended the new # operator to the list of symbol expressions in the build script. I don't believe the current code will correctly handle it's higher operator precedence though.

@coveralls
Copy link

coveralls commented Jul 10, 2018

Coverage Status

Coverage decreased (-0.2%) to 98.426% when pulling e37b115 on SilentSpike:update-database into 2c826d7 on LordGolias:master.

@kymckay
Copy link
Contributor Author

kymckay commented Jul 10, 2018

Added some tests to validate the precedence, wasn't entirely sure where they fit so please move as necessary

@LordGolias
Copy link
Owner

LordGolias commented Jul 10, 2018

Thanks for the PR!

To clarify, is # a new operator in SQF? If yes, do we know its precedence? What does it mean?

Secondly, the other commands, were they added by running build_database_with_returns.py? If yes, can we split the commit in 2, since the new operator # is more than just a new command. :P

The precedence is set in the file parser_exp.get_lbp. Depending on the precedence of the operator, we must add it to the respective if branch.

@jonpas
Copy link
Contributor

jonpas commented Jul 10, 2018

It is a new operator, it is same as select except it has higher precedence (unsure where it fits in exactly).

@kymckay
Copy link
Contributor Author

kymckay commented Jul 10, 2018

Yeah it's strictly array # index and has higher precedence than math operators

@kymckay
Copy link
Contributor Author

kymckay commented Jul 10, 2018

Will remove the other commands from this PR (do you want to just run the script and add them as a commit?)

@kymckay kymckay changed the title Update database.py with new commands Add new hash operator with precedence Jul 10, 2018
@kymckay
Copy link
Contributor Author

kymckay commented Jul 10, 2018

Did some testing and verified that the precedence is above math operators, but below unary.

Thought I'd added the necessary line to parser_exp.get_lbp to fix it, but it seems to not be the case 🤔

@LordGolias
Copy link
Owner

I've pushed a commit that fixes both tests. Essentially,

  1. the operator also needs to be in the tokenizer or #1 is interpreted as a variable
  2. the functionality of the operator needs to be added to the interpreter (same as select)

There is a new branch, https://github.com/LordGolias/sqf/tree/SilentSpike-update-database that contains your commits plus the commit I've made. Feel free to squash all of those in a single commit, and PR it if you feel that this is the intention of this PR.

@dedmen
Copy link

dedmen commented Jul 11, 2018

#'s precedence is 10. Where select is only 4.

Soo https://github.com/LordGolias/sqf/pull/37/files#diff-6e0dcbb86211d8b08ead9aee3b511f54R47 is not quite correct.

@kymckay
Copy link
Contributor Author

kymckay commented Jul 11, 2018

@dedmen How come alive [objNull]#0 errors in game though? Does alive have higher precedence? (Edit: You can see unary operators currently have precedence 9)

@dedmen
Copy link

dedmen commented Jul 11, 2018

https://community.bistudio.com/wiki/SQF_syntax#Rules_of_Precedence That list is wrong too though.. Meh.. So hard finding reliable sources -.-

Binaries are by default prio 4
Unaries should be 8
^ should be 9.
And there are no commands with 10. Besides the recently added #

I don't know why things happen. I'm reading these values directly from the 1.82 binary.

Here https://gist.github.com/dedmen/4a910ee95d7d4c0a7cc70c6435d93c6e
direct copy paste from 1.82 binary. the 3rd argument is the priority.

@kymckay
Copy link
Contributor Author

kymckay commented Jul 11, 2018

  • ^ precedence can be changed in a separate PR
  • This PR is actually going to be a bit more complicated than anticipated because # and ## are also valid preprocessor commands
    • Presumably \#\# also needs to be added to the base tokenizer in front of \#include
    • Other than that, correctly handling those is probably beyond my current understanding of the codebase here
  • The actual # sqf command should now be correctly implemented in this PR (little unclear on how the precedence differs from the engine code, but seems to match observed behaviour - can do some more in-game testing)

@kymckay
Copy link
Contributor Author

kymckay commented Jul 11, 2018

Demonstration of the in-game precedence:

2018-07-11_15-30-13

2018-07-11_15-33-25

@kymckay
Copy link
Contributor Author

kymckay commented Jul 11, 2018

So it seems that adding the correct precedence of 10 somehow still works as expected (even though the unary operators have lower precedence). Have now done so and cleaned up the commit history 👍

Just need to now figure out how this breaks usage of ## and # preprocessor commands (I wonder how the engine knows the difference between SQF # and preprocessor # 🤔)

@jonpas
Copy link
Contributor

jonpas commented Jul 11, 2018

If there is anything in front of # then it's SQF (you can't have # 5;), otherwise preprocessor?

@dedmen
Copy link

dedmen commented Jul 11, 2018

preprocessor can have stuff in front if you are inside a macro. Guess that doesn't matter though?

@kymckay
Copy link
Contributor Author

kymckay commented Jul 11, 2018

Did some preliminary testing to determine the behaviour of ## and # preprocessor commands (via compile preProcessFile). My prediction was that the preprocessor behaviour would match that of c++. Which seems to mostly be true.

What works

// Concatenation works for static macros
#define test global##Var
A; // globalVar

// Stringification works for static macros
#define A #index
A; // "index"

// Concatenation behaviour with two stringified variables
#define test(var1,var2) #var1###var2
test(concat,strings); // "concat""strings"

What doesn't work

"concat"##"strings"

Error Invalid number in expression

Clearly the ## operator only works in macro definitions

#stringify

No error shown, but compiled function is empty (this seems to happen if there's a # followed by a word character anywhere in the file that isn't a macro definition - presumably because the preprocessor can't finish processing). Conclusion: # operator also only works in macro definitions

#define A #![]{}/.,
A;

Error Missing ;

It seems like you can only stringify word characters (\w in regex speak). Bonus points: this one is extra weird, for some reason if it outputs: "!"[]{}/.,; with the word test at the front; "huh"![]{}/.,; with the word huh at the front; and just the symbols without quotation when no word is at the front like shown above.

Edit: Some clarification on this one, it seems to follow the same rules as variable names. The first character has to be a word character, but digits can also be used elsewhere.

#define test(index) [0,1,2] # index

test(1)

Error Missing ;

It seems the sqf # command just can't be used in a macro definition

#define concat(var1,var2) var1 ## var2
concat(1,2);

Error Missing ;

Concatenation doesn't remove all space separation the same as specified by C++ doc

Simply replace instances of the SQF # Keyword within macro definitions
with an instance of the Preprocessor variant
@kymckay
Copy link
Contributor Author

kymckay commented Jul 12, 2018

Slowly developing my understanding of the codebase here. I believe I've now got the parser correctly identifying both ## and # preprocessor commands.

Just need to now make sure it knows how to handle their usage. I think once this PR is ready it will additionally fix #34 (edit: only in cases where the macro is defined in the same file, would need to fully handle #include otherwise)

@kymckay
Copy link
Contributor Author

kymckay commented Jul 12, 2018

Stringification is now correctly resolved by the parser for simple cases. However, it seems that whenever multiple tokens compose a macro argument it silently breaks down.

e.g.

#define quote(a) #a

quote(Space here) // Doesn't work
quote(%) // Works
quote(%%) // Doesn't work

@kymckay
Copy link
Contributor Author

kymckay commented Jul 13, 2018

I believe the concatenation test is failing because of the way I'm performing it with parse() resulting in the wrong nesting of Statements

e.g.

expected = parse('\nsomegvar')
expected.tokens # [S<<EOL>V<somegvar>>]

p = parse('#define gvar global##Var\ngvar')
p[1].result # S<<EOL>S<S<V<globalVar>>>>

@kymckay
Copy link
Contributor Author

kymckay commented Jul 13, 2018

I'm going to leave concatenation for the time being and focus on fixing more advanced uses of stringification. The issue I've run into is that the ## token is inside a Statement so the way I've implemented it correctly uses the statement (or token) to the left, but incorrectly only joins it with the token to the right (instead of the rest of the whole statement). This means that when the result is re-parsed you get a statement within a statement.

@kymckay
Copy link
Contributor Author

kymckay commented Jul 13, 2018

Yeah, I think concatenation will have to be implemented differently than how I've done it. Re-parsing has the potential to produce statements, so when an instance of the macro is replaced in the code, it has extra nested statements which shouldn't be there.

I may write a bunch of tests for these operators and then hand this one over to you @LordGolias

@kymckay
Copy link
Contributor Author

kymckay commented Jul 13, 2018

🤔 I think there's actually already a bit of a discrepancy between the way statements are nested when it comes to macros being replaced in code. It's possible I'm just getting confused by all the nesting though:

p = parse('\n"string"')
p.tokens # [S<<EOL>s<"string">>]

p = parse('#define string "string"\nstring')
p[1].result.tokens # [<EOL>, s<"string">] <- Missing outer statement

Edit: I think I see, the result takes the place of the outer statement?

@kymckay
Copy link
Contributor Author

kymckay commented Jul 16, 2018

@LordGolias This PR has very quickly increased in scope (sadly necessary) to the point where parser implementation decisions are needed that are probably more suited to your knowledge. So I'm going to go hands off from here on.

On the bright side I'm now pretty familiar with the code. Also, just to note something I didn't get round to testing for arma's preprocessor, but I assume it works the same

Edit: If you want to create a dev branch I can re-target this PR at it so you want to work off the top of it. Also, I'm pretty sure my previous comment is actually a minor bug in how macros are resolved by the parser.

@kymckay
Copy link
Contributor Author

kymckay commented Sep 4, 2018

Considering revisiting this, here's an interesting read on how the C Preprocessor handles things for inspiration.

@kymckay
Copy link
Contributor Author

kymckay commented Aug 15, 2021

Old PR I don't have time to revisit. Unsure if this was ever the right approach for this project anyway.

@kymckay kymckay closed this Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants