Skip to content

perf(windows): fold shell option once#2605

Open
CatsDeservePets wants to merge 1 commit into
gokcehan:masterfrom
CatsDeservePets:windows-cmd
Open

perf(windows): fold shell option once#2605
CatsDeservePets wants to merge 1 commit into
gokcehan:masterfrom
CatsDeservePets:windows-cmd

Conversation

@CatsDeservePets

Copy link
Copy Markdown
Collaborator

No description provided.

@CatsDeservePets

Copy link
Copy Markdown
Collaborator Author

Hello @joelim-work!

Any thoughts on this? I would not even call it a micro optimisation.

When comparing compiler optimisations for quoteString, gopls previously returned:
cannotInlineFunction(function too complex: cost 131 exceeds budget 80) [windows,amd64]
Now it states the following:
canInlineFunction(cost: 72) [windows,amd64]

I just don't know whether this should be mentioned inside the changelog. In theory, this might causes regressions if users relied on checks for the shell and on purpose wrote it in uppercase letters (even though lf uses cmd everywhere) and only ever checked for CMD. However, due to differences in shell syntaxes, I can't think of any cross platform scripts that do checks like this though.

@joelim-work

Copy link
Copy Markdown
Collaborator

I guess there's three choices, each of them has some kind of drawback:

  1. Do not normalize gOpts.shell and compare uppercase/lowercase every time (less efficient)
  2. Normalize gOpts.shell to lowercase (will affect things like the lf_shell environment variable and {{Options.shell}} in the ruler)
  3. Do not allow case insensitivity at all and ask users to specify cmd only in lowercase (breaking change)

TBH I'm very indifferent about this and am happy to leave the decision to you.

@joelim-work

Copy link
Copy Markdown
Collaborator

I guess there's three choices, each of them has some kind of drawback:

  1. Do not normalize gOpts.shell and compare uppercase/lowercase every time (less efficient)
  2. Normalize gOpts.shell to lowercase (will affect things like the lf_shell environment variable and {{Options.shell}} in the ruler)
  3. Do not allow case insensitivity at all and ask users to specify cmd only in lowercase (breaking change)

Sorry I realized there's another approach in addition to the above:

  1. Add a new global variable to track whether Windows CMD is being used as the shell, this decouples the logic from the gOpts.shell setting (slightly more complexity)

Maybe this is the least evil of the possible solutions.

@CatsDeservePets

Copy link
Copy Markdown
Collaborator Author

I guess there's three choices, each of them has some kind of drawback:

  1. Do not normalize gOpts.shell and compare uppercase/lowercase every time (less efficient)
  2. Normalize gOpts.shell to lowercase (will affect things like the lf_shell environment variable and {{Options.shell}} in the ruler)
  3. Do not allow case insensitivity at all and ask users to specify cmd only in lowercase (breaking change)

Sorry I realized there's another approach in addition to the above:

4. Add a new global variable to track whether Windows CMD is being used as the shell, this decouples the logic from the `gOpts.shell` setting (slightly more complexity)

Maybe this is the least evil of the possible solutions.

I thought about this as well. But this is making things more complex than it should be in my opinion. Maybe I am thinking way too deep about such a simple thing which may not be worth changing anyway.

@joelim-work

Copy link
Copy Markdown
Collaborator

Windows CMD doesn't behave like other shells so there's no way to avoid some kind of complexity as long as it is supported. TBH I don't have a strong opinion on which approach is used, or if the current implementation needs to be changed at all. Although I think I would prefer not to modify the value stored in gOpts.shell since that affects other things like lf_shell and the ruler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants