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

perf!: don't look for indent/dedent if comment is at current indent level #244

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

llllvvuu
Copy link

@llllvvuu llllvvuu commented Sep 22, 2023

Whether an indent/dedent token is generated for a comment depends on
whether the indent/dedent is persisted after the comment. This takes
O(comment_length) to check. Checking it for every line takes
O(comment_length * comment_lines), which is quadratic.

An optimization was made in a901729 which skips this check when there
is no possible indent/dedent, such as in the following:

# comment 1...
# comment 1000
print("foo")

However, the check cannot be skipped in the following case:

class Foo:
    def foo():
        print("bar")
    # comment 1...
    # comment 1000
    def bar():
        print("foo")

which comes up when commenting out code.

This PR optimizes this case by skipping the check if the comment is at the
current indent length. In the above example, # comment 1 looks ahead
to def bar() and changes the indent length to 4, after which comments
2-1000 do not look ahead, since they are also at indent length 4.

This PR does not optimize the following case:

class Foo:
    def foo():
        print("bar")
    # comment 1...
    # comment 1000
        print("foo")

but it does optimize:

class Foo:
    def foo():
        print("bar")
        # comment 1...
        # comment 1000
        print("foo")

BREAKING CHANGE: In the following scenario, we previously generated an
indent token before comment 1; we now generate it before comment 2.
This is more consistent with how dedents are handled.

def foo():
# comment 1
   # comment 2
   print("bar")

Related: nvim-treesitter/nvim-treesitter#4839

@llllvvuu llllvvuu force-pushed the wip/double_comment_2 branch 2 times, most recently from 73ab0ae to b452dd3 Compare September 22, 2023 05:45
@llllvvuu llllvvuu changed the title WIP!: alternate solution for redundant comment scan WIP!: simpler solution for redundant comment scan Sep 22, 2023
@llllvvuu llllvvuu changed the title WIP!: simpler solution for redundant comment scan perf!: don't look for indent/dedent if comment is at current indent level Sep 22, 2023
@llllvvuu llllvvuu force-pushed the wip/double_comment_2 branch 2 times, most recently from b3e8db5 to 34108b4 Compare September 22, 2023 22:32
@llllvvuu llllvvuu marked this pull request as ready for review September 22, 2023 22:33
@llllvvuu llllvvuu force-pushed the wip/double_comment_2 branch 2 times, most recently from c042932 to 9ff33c8 Compare September 22, 2023 22:38
…evel

Whether an indent/dedent token is generated for a comment depends on
whether the indent/dedent is persisted after the comment. This takes
O(comment_length) to check. Checking it for every line takes
O(comment_length * comment_lines), which is quadratic.

An optimization was made in `a901729` which skips this check when there
is no possible indent/dedent, such as in the following:
 ```python
 # comment 1...
 # comment 1000
 print("foo")
 ```
However, the check cannot be skipped in the following case:

```python
class Foo:
    def foo():
        print("bar")
    # comment 1...
    # comment 1000
    def bar():
        print("foo")
```
which comes up when commenting out code.

This PR optimizes this case by skipping the check if the comment is at the
current indent length. In the above example, `# comment 1` looks ahead
to `def bar()` and changes the indent length to 4, after which comments
2-1000 do not look ahead, since they are also at indent length 4.

This PR does not optimize the following case:

```python
class Foo:
    def foo():
        print("bar")
    # comment 1...
    # comment 1000
        print("foo")
```

but it does optimize:

```python
class Foo:
    def foo():
        print("bar")
        # comment 1...
        # comment 1000
        print("foo")
```

BREAKING CHANGE: In the following scenario, we previously generated an
indent token before comment 1; we now generate it before comment 2.
This is more consistent with how dedents are handled.

 ```python
 def foo():
 # comment 1
    # comment 2
    print("bar")
 ```
@ahlinc
Copy link

ahlinc commented Sep 27, 2023

What's interesting is that before the 188b6b06 commit it seems there was no such problem with comments, so maybe it worth to looking for an alternative solution and try to remove comments from externals.

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.

None yet

2 participants