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

⚙️ __AS=$? on every command also if the status isn't checked #183

Open
Mte90 opened this issue Jun 12, 2024 · 6 comments
Open

⚙️ __AS=$? on every command also if the status isn't checked #183

Mte90 opened this issue Jun 12, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@Mte90
Copy link
Member

Mte90 commented Jun 12, 2024

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
Copy link
Member Author

Mte90 commented Jun 12, 2024

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

@Ph0enixKM
Copy link
Collaborator

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

@Mte90
Copy link
Member Author

Mte90 commented Jun 17, 2024

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

@Ph0enixKM Ph0enixKM changed the title [Bug] __AS=$? on every command also if there is unsafe ⚙️ __AS=$? on every command also if there is unsafe Jun 19, 2024
@Mte90 Mte90 added the enhancement New feature or request label Jun 24, 2024
@Mte90
Copy link
Member Author

Mte90 commented Jul 4, 2024

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
Copy link
Member Author

Mte90 commented Jul 16, 2024

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 Mte90 changed the title ⚙️ __AS=$? on every command also if there is unsafe ⚙️ __AS=$? on every command also if the status isn't checked Jul 16, 2024
@Mte90
Copy link
Member Author

Mte90 commented Jul 16, 2024

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants