Nested DllCall conversion

[eugenesvk]

v1
DllCall("oleacc\AccessibleChildren", "Ptr", ComObjValue(Acc), "Int", 0, "Int", cChildren, "Ptr", VarSetCapacity(varChildren,cChildren*(8+2*A_PtrSize),0)*0+&varChildren, "Int*", cChildren)

v1→v2 converter
DllCall("oleacc\AccessibleChildren", "Ptr", ComObjValue(Acc), "Int", 0, "Int", cChildren, "Ptr", varChildren := Buffer(cChildren*(8+2*A_PtrSize), 0)*0+&varChildren, "Int*", &cChildren := 0)

Shouldn't
varChildren := Buffer(cChildren*(8+2*A_PtrSize), 0)*0 +&varChildren instead be
(varChildren:= Buffer(cChildren*(8+2*A_PtrSize), 0))*0+&varChildren?

[mmikeww]

Yes I believe it should be.

If you want to take a crack at fixing that, feel free. Lol. Pull requests accepted!

[eugenesvk]

Sorry, I barely understand this DllCalls and Buffer etc syntax as is and struggle if my copy&pasted examlpes misbehave, unlikely would be able to help much in writing a translation code to fix it :(

I might take a look at it as it might be as simple as adding *0 to a copy of VarSetCapacity(TargetVar,RequestedCapacity,FillByte) | *_VarSterCapacity and copy&pasting the _VarSterCapacity function to put the parenthesis in the correct places

Just to understand your testing framework — is there currently a way to test the files directly, e.g. VarSetCapacity_ex1.ah1 and VarSetCapacity_ex1.ah2 (that would be extremely helpful as it's easy to delete everything, modify these two files and see if I can change the rules to make it work, then get the other tests and see if anything is broken)
or is the only way to work is to paste the content to the Tests.ahk file?

[mmikeww]

The original way I did the testing framework was to use the Tests.ahk file

But @dmtr99 added his new way of testing using the ah1 and ah2 files with the QuickConvertor script. You can try running that and playing around with it. I'm not exactly sure how it works to be honest

[eugenesvk]

I've finished the tests that adds extra parenthesis to match the example, they passed, but then setting this.test_exec := true revealed that the suggested "fixed" conversion is wrong — you can't multiply a buffer object by 0, you get an Expected a Number, but got a Buffer. runtime error, so the proper way to fix this is something that should've been done in the original code to make it at least somewhat readable — you need to set the var outside the DllCall and then use it as a parameter

So I guess there is no point in submitting the fix, this example should just manually be rewritten

Let me know if you feel the fix is still useful for something and I'll submit it anyway, just disabling the FileAppend part so that it wouldn't be actually executed in tests

[mmikeww]

I saw your attempted fix in your branch, I added a comment there.

The proper fix would be complicated, and that would be to check if the replacement is inside another expression (which i have no idea how we'd determine that), and then if so, add parens around the whole replacement.

Otherwise, maybe a simpler fix that would work for this specific case, would be to change this line:

AHK-v2-script-converter/ConvertFuncs.ahk

Line 3162 in 11a0d14

Return Format("{1} := Buffer({2}, {3})", p*)

Return Format("{1} := Buffer({2}, {3})", p*)

to

Return Format("({1} := Buffer({2}, {3}))", p*)

but that will end up always putting parens around all of these replacements, even if they are on a line by themselves

[eugenesvk]

but that will end up always putting parens around all of these replacements, even if they are on a line by themselves

Yeah, that's what my "fix" actually does, and I haven't noticed that since there was not a unit test for the non-*0 version of the VarSetCapacity function call and thought your parser did some magic by matching ()*0 separately from () in the FunctionsToConvert

check if the replacement is inside another expression (which i have no idea how we'd determine that), and then if so, add parens around the whole replacement.

maybe that's the proper general way, but for this can't we "just" change the parser not to finish at the ending ), but also match everything after that (within the given string VarSetCapacity(TargetVar,RequestedCapacity,FillByte)*0) and pass it as a separate {4} parameter to the _VarSterCapacity function so that the function can know it's not blank and you need to do something different

[safetycar]

If you don't mind another opinion in the thread...
I think this case could be generalized to other functions.
Probably a proper check to add wrapping parenthesis would go just before this line:
https://github.com/mmikeww/AHK-v2-script-converter/blob/master/ConvertFuncs.ahk#L604
My general idea is that if "NewFunction" came back with an assignment ":=", and creating some more checks to the context inside oResult.Pre and oResult.Post there would be some clue if it's worth to wrap it in parenthesis.
But I haven't thought that much on what the context checks would be.

[eugenesvk]

Check out this new hack, that should force function(p)_extra into submission!
(also moved some data to separate files as the core file is huge enough and converted into an easier2parse OrderedMap format)
Now the () seem to guard only the multiplied functions, not all of them like before

[eugenesvk]

But I haven't thought that much on what the context checks would be.

yeah, that's the hard part ;) maybe @mmikeww knows AHK syntax enough to set the proper rules

[mmikeww]

(also moved some data to separate files as the core file is huge enough and converted into an easier2parse OrderedMap format)

Do we really need the OrderedMap? A quick look and it doesn't seem to offer any real benefits. You could've just moved the strings to other files
 

Check out this new hack, that should force function(p)_extra into submission!

This is great work. As long as all the existing unit tests pass, I'm fine with merging it. Overall, this project is just a bunch of hacks thrown together. That's why the unit tests are so valuable, because we can easily see if we broke something that used to work.

Only suggestion is I think you should merge the tests commit with the implementation commit, so that they are one distinct commit.
 

Now the () seem to guard only the multiplied functions, not all of them like before

Could this:
VarSetCapacity(TargetVar,RequestedCapacity,FillByte)\*\d*
be changed to:
VarSetCapacity(TargetVar,RequestedCapacity,FillByte)[\+\-\*\\/]\d*
to cover +,-,*,/
 

If you don't mind another opinion in the thread... I think this case could be generalized to other functions. Probably a proper check to add wrapping parenthesis would go just before this line: https://github.com/mmikeww/AHK-v2-script-converter/blob/master/ConvertFuncs.ahk#L604 My general idea is that if "NewFunction" came back with an assignment ":=", and creating some more checks to the context inside oResult.Pre and oResult.Post there would be some clue if it's worth to wrap it in parenthesis. But I haven't thought that much on what the context checks would be.

Yes very quickly, we can start to question, can this work for more expressions? I think you are on the right path with your idea. Whereas eugene just tried to solve a very specific case, your plan could be generalized for many more.

Sadly, I do not really have the time currently to revisit this project. If you guys are capable of implementing this stuff, along with associated unit tests, and all existing tests pass, then I'm happy to merge the changes in.

[eugenesvk]

Do we really need the OrderedMap? A quick look and it doesn't seem to offer any real benefits. You could've just moved the strings to other files  

Well, then we'd have to worry about properly splittng on "|" and whether some regex rule broke it silently by adding it's own |, so strings aren't a proper data structure for this, why bother splitting after if we already know the proper split before?
We could use Maps(1, ["NameV1","NameV2"]) to avoid the dependency, but then not sure the order of this map is guaranteed by AHK, so might be a bit brittle

As long as all the existing unit tests pass ... That's why the unit tests are so valuable, because we can easily see if we broke something that used to work.

Fully agree on the value of unit tests. I only have one broken test StringReplace_All_NoReplaceText, but it was broken for me on the master as well, so not relate to these changes

Only suggestion is I think you should merge the tests commit with the implementation commit, so that they are one distinct commit.  

Merged

Could this: VarSetCapacity(TargetVar,RequestedCapacity,FillByte)\*\d* be changed to: VarSetCapacity(TargetVar,RequestedCapacity,FillByte)[\+\-\*\\/]\d* to cover +,-,*,/

Maybe? In general, this doesn't make much sense due to the change in types:
in v1 the output of was GrantedCapacity integer, so you could potentially + or - to this, but
in v2 the output is a buffer, for which these operations don't make sense.
But then this is no different from the original * :) and they seem to both raise a runtime error on use, so not sure which ways is less bad ;)

[mmikeww]

Do we really need the OrderedMap? A quick look and it doesn't seem to offer any real benefits. You could've just moved the strings to other files

Well, then we'd have to worry about properly splittng on "|" and whether some regex rule broke it silently by adding it's own |, so strings aren't a proper data structure for this, why bother splitting after if we already know the proper split before? We could use Maps(1, ["NameV1","NameV2"]) to avoid the dependency, but then not sure the order of this map is guaranteed by AHK, so might be a bit brittle

True. Before we weren't using regexmatches I don't believe, so there was no concern about the stray pipe. I think your solution was the first to use a regex.

Could this: VarSetCapacity(TargetVar,RequestedCapacity,FillByte)\*\d* be changed to: VarSetCapacity(TargetVar,RequestedCapacity,FillByte)[\+\-\*\\/]\d* to cover +,-,*,/

Maybe? In general, this doesn't make much sense due to the change in types: in v1 the output of was GrantedCapacity integer, so you could potentially + or - to this, but in v2 the output is a buffer, for which these operations don't make sense. But then this is no different from the original * :) and they seem to both raise a runtime error on use, so not sure which ways is less bad ;)

Good point, but then, if the conversion is invalid, why should I converter give this as a result? In other instances, we would insert a comment line with some explanation instead of giving false conversion

[eugenesvk]

Good point, but then, if the conversion is invalid, why should I converter give this as a result? In other instances, we would insert a comment line with some explanation instead of giving false conversion

Well, the conversion is valid (in the case *0), it just can't be inlined like before, needs to be moved before (and not zeroed out anymore). By the way, is it actually possible with this parser to move the result to a previous separate line (then we could also remove the *0)?
Otherwise, could you please point to an example of a function where a comment is inserted on a separate line, might be a good idea to add to this?

Re. the other + / -, wouldn't those only be used with a 2-parameter function for strings, which would be converted to VarSetStrCapacity and not suffer from a :=
And in they are used with a 3-parameter VarSetCapacity for some reason then let's also just add a comment

[mmikeww]

Well, the conversion is valid (in the case *0)

Should we only convert this valid case then?

By the way, is it actually possible with this parser to move the result to a previous separate line (then we could also remove the *0)?

Initially there was no support for moving replacements on previous/next lines, but I don't recall if @dmtr99 added this feature when he was overhauling the converter. However....

Otherwise, could you please point to an example of a function where a comment is inserted on a separate line, might be a good idea to add to this?

There are cases where we simply replace multiple lines instead of one. The output string just includes `n chars. Example of comment:

AHK-v2-script-converter/ConvertFuncs.ahk

Line 3026 in 11a0d14

Out := comment Indentation . format("{1} := StrReplace({2}, {3},,,, 1)", p*)

Example of multi line replace:

AHK-v2-script-converter/ConvertFuncs.ahk

Lines 3177 to 3181 in 11a0d14

	Out := format("o{1} := WinGet{2}({3},{4},{5},{6})", p*) "`r`n"
	Out .= Indentation "For v in o" P[1] "`r`n"
	Out .= Indentation "{`r`n"
	Out .= Indentation " " P[1] " .= A_index=1 ? v : `"``r``n`" v`r`n"
	Out .= Indentation "}"

I believe these have to go in the custom replacement funcs

Re. the other + / -, wouldn't those only be used with a 2-parameter function for strings, which would be converted to VarSetStrCapacity and not suffer from a :=
And in they are used with a 3-parameter VarSetCapacity for some reason then let's also just add a comment

I don't know, I haven't kept up with this project and won't have time in the near future. If you want to take some initiative, I can give you write access to the repository as well, like i did with dmtr99

[eugenesvk]

Should we only convert this valid case then?

That was the my idea behind only the * support, but I've recently encountered varsetcap()//4 where the granted capacity result of varsetcapactiy was actually used in a loop in a legitimate way to get the whole number of 4-byte vars, so your idea of adding more operators makes more sense, so instead of trying to guess, which of those operators might be valid or not (unless you know, so please do tell :) ), I guess we should have a proper generalized conversion, though this should require us to:

• realize that we don't really know its a buffer even with 3 args (I've seen strings for WinAPIs that should be strings that had 3 args in v1, don't know enough whether you really needed to zero them out, but at any rate that's a valid v1 use case that shouldn't be buffered in v2)
• remove the all-too-smart inlining and set var as a buffer/string on a previous line
• use buffer.Size in place of where varsetcapacity was (unless its a string, then just use the granted capacity from the previous line)

So the rule should be something like this:

v1 VarSetCapacity(vTarget , RequestedCap, FillByte) +,-,/,*,// X
v2

• line − 3: ; Warning that there are two options: (usually) a buffer and (sometimes) a string, it depends on which API the variable is used in and we don't have the capacity to guess it
• line − 2: vTarget := Buffer(RequestedCap, FillByte) ; option 1
• line − 1: vTargetCap_v1v2 := VarSetStrCapacity(&vTarget, RequestedCap) ; option 2, replace vTarget with vTargetCap_v1v2 below
• conversion line: vTarget + - / * // X

OR in the absence of a previous-line-comment feature we can use this hack:
v1 VarSetCapacity(vTarget , RequestedCap, FillByte) +,-,/,*,// X
v2 (vTarget:=Buffer(RequestedCap, FillByte)).Size +,-,/,*,// X
This also sets the variable and uses its size inline (but doesn't prevent mistakes when vTarget was supposed to be a string)

So I'll implement this hack then and leave the proper commented conversion for when there is a previous-line-comment feature

There are cases where we simply replace multiple lines instead of one.

Yeah, unfortunately, this wouldn't work in complex inline cases as we don't really know all the contexts in which they can appear (it's not just DllCalls, but also loops, but then also I have no clue what else) :(

Re. the other + / -, wouldn't those only be used with a 2-parameter function for strings

Likely nope as I've seen with the // example mentioned above :(

If you want to take some initiative, I can give you write access to the repository as well, like i did with dmtr99

Thanks, though given my limited knowledge of the syntax and ahk language, I'd prefer if someone reviews the changes

[eugenesvk]

@dmtr99 have you by any chanced added a feature to display additional commens on a new previous line during conversion so that we can signal some warnings to the user?

[dmtr99]

@dmtr99 have you by any chanced added a feature to display additional commens on a new previous line during conversion so that we can signal some warnings to the user?

No, you could add it to the line itself, or adding an extra global variable in the line:
ScriptOutput .= Line . EOLComment . "rn"

Note: I would certainly test it if this does not cause errors for multi-line commands.

[eugenesvk]

adding an extra global variable in the line

Thanks, this seems to work! I reset it to "" right after this line, so this should have no side effects, right, since this line is called per "parsing unit", so it would be called only once for each function?

By the way, is the whole line available to me inside the conversion function (e.g. _VarSetCapacity(p) {}) so that I can parse it and see if it e.g. ends in a line continuation symbol or something?

[mmikeww]

Inside the custom func replacements themselves (that we've prefixed with _ ), the whole line is not available, only the parameters that we parsed earlier which is passed in with the array p*

But you can preceed the output line with a comment, by just doing something like:

return "comment here`r`n" . Format(replacement output line)

[eugenesvk]

Inside the custom func replacements themselves (that we've prefixed with _ ), the whole line is not available, only the parameters that we parsed earlier which is passed in with the array p*

I think I can partially get around that by passing oResult as an argument, then at least I can get everything except for the comment in e.g. oResult.Post
Line comments for some reason are not part of some oResult property, might need to add them as then I'll definitely be certain that it's safe to add another comment there

But you can preceed the output line with a comment, by just doing something like:

I don't think I can since my Format() isn't the full line, it can be a function argument.
But I can do what dmtr99 suggested and change a global var

[eugenesvk]

Ok, think I've done everything structural related to VarSet, now it can add new buffers to new previous lines, zero out with ().Size where possible, warn that any conversion can be wrong :) and suggest the alternative in an end-of-line comment etc., and now DllCall is even a bit smarter and knows how to guard the resulting VarSetStr in a StrPtr()!

I've added a warning than if there is an un{guarded} control flow statement and there is a new line(s) created so that the user should guard it manually, but can the parser track it?
That is, can I know that the previous line was a control flow statement and it ended without {?
Then I'd be able to add {} myself

[mmikeww]

That is, can I know that the previous line was a control flow statement and it ended without {?

I don't believe our parser can track that

[eugenesvk]

Can the parser convey this info about the current line?
Then I guess we could save it in some wasPreviousLineControlNoBlock :-) global variable and act accordingly in parsing the current line

[mmikeww]

I don't believe we even track control flow statements as a single group

The if statements are all handled near the top of the conversion function and detected with regexes

The IfCommands and Loop statements are handled separately in the long list of Command parsing