Amber icon indicating copy to clipboard operation
Amber copied to clipboard

[Bug] __AS=$? on every command also if the status isn't checked

Open Mte90 opened this issue 1 year ago • 7 comments

So the amber script is:

silent unsafe {
    $cd lua-language-server$
    $git pull$
    $./make.sh$
    $ln -s /opt/lua-language-server/bin/lua-language-server /usr/local/bin/lua-language-server$
}

Generate this bash script:

cd lua-language-server >/dev/null 2>&1
__AS=$?
git pull >/dev/null 2>&1
__AS=$?
./make.sh >/dev/null 2>&1
__AS=$?
ln -s /opt/lua-language-server/bin/lua-language-server /usr/local/bin/lua-language-server >/dev/null 2>&1
__AS=$?
cd /tmp >/dev/null 2>&1
__AS=$?

Create that variable also if not used is not helpful at all in the bash script generated. I think that if unsafe it is used that variable shouldn't be added.

Mte90 avatar Jun 12 '24 14:06 Mte90

I think that should be enough to check on https://github.com/Ph0enixKM/Amber/blob/69e96ae3867091bcd374ff3cf42909324e73646e/src/modules/expression/literal/status.rs#L28 if the command is unsafe to not print that

Mte90 avatar Jun 12 '24 15:06 Mte90

But if developer uses unsafe modifier does this imply that they don't want to check the status later on? @Mte90

I think this should be a part of rendered code optimisation for Amber

Ph0enixKM avatar Jun 17 '24 13:06 Ph0enixKM

If the status is not used after why add code anyway? This is kind of a compiler optimization to generate only useful code.

Mte90 avatar Jun 17 '24 15:06 Mte90

So checking my experimental script to evaluate improvements to Amber https://github.com/Mte90/My-Scripts/blob/master/dev/lsp-installer/install.sh

We can see as today tons of __AS=$? that are useless.

Mte90 avatar Jul 04 '24 10:07 Mte90

So https://github.com/amber-lang/amber/blob/master/src/modules/condition/failed.rs#L95 adds the useless variable. I think that this code should check if the next line in the amber code is checking for the status variable.

Mte90 avatar Jul 16 '24 16:07 Mte90

Maybe the approach should be the apposite looking at the code.

We add __AS=$? basically at every command, instead when it is called status we should add it the line before.

We can't do it on https://github.com/amber-lang/amber/blob/master/src/modules/expression/literal/status.rs#L30 as it is just a replace so how we can do that (I am saying just to understand how works)?

Mte90 avatar Jul 16 '24 16:07 Mte90

The solution that I came up with is recorded on this video: https://www.youtube.com/watch?v=_caXIMxrYj8 We could implement this optimization for this issue

Ph0enixKM avatar Jul 18 '24 21:07 Ph0enixKM