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

Properly show symbol kinds and structs/modules in document outline #98

Merged
merged 1 commit into from Mar 20, 2023

Conversation

mwpenny
Copy link
Contributor

@mwpenny mwpenny commented Mar 20, 2023

While trying to understand the details of DocumentSymbolProvider, I found these small mistakes.

  1. If a regex for macros is defined (true for all language IDs other than asm-list-file) then processing of modules, structs, and symbol kinds is skipped.

    • Before
      image
    • After
      image
  2. If a label immediately follows a constant or data symbol, then the wrong symbol kind is used.

    • Example
      image
    • Before
      image
    • After
      image

For the screenshots, I used tests/modulesstruct.asm.

@@ -114,8 +113,8 @@ export class DocumentSymbolProvider implements vscode.DocumentSymbolProvider {
const range = new vscode.Range(line, 0, line, 10000);
const macroSymbol = new vscode.DocumentSymbol(macroName, '', vscode.SymbolKind.Method, range, range);
symbols.push(macroSymbol);
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than continueing if the regex is defined, the continue now only happens if there was actually a match.

@@ -186,9 +183,8 @@ export class DocumentSymbolProvider implements vscode.DocumentSymbolProvider {
elem.kind = kind;
elem.detail = match![1] + ' ' + match![2].trimEnd();
}
defaultSymbolKind = kind;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating defaultSymbolKind seems unnecessary since this code will always go back and update all lastSymbols if required anyway. Let me know if this is incorrect.

Removing defaultSymbolKind also simplifies the code a bit.

@maziac
Copy link
Owner

maziac commented Mar 20, 2023

Your PR makes sense. Seems that I broke my code some time.
I would like to remember what the reason for this code was:

                    // Check if found
                    if (kind != undefined) {
                        // It's something else than code
                        for (const elem of lastSymbols) {
                            elem.kind = kind;
                            elem.detail = match![1] + ' ' + match![2].trimEnd();
                        }
                        defaultSymbolKind = kind;
                    }

Seems that there was some special implementation for constants (equ).

@maziac maziac merged commit 11dd14d into maziac:main Mar 20, 2023
@maziac
Copy link
Owner

maziac commented Mar 20, 2023

Ahh. Sorry, I forgot to ask:
Is your PR under MIT license?

@mwpenny
Copy link
Contributor Author

mwpenny commented Mar 20, 2023

Yes, I agree that my code can be released under the same license as the rest of this project (MIT).

@maziac
Copy link
Owner

maziac commented Mar 20, 2023

Thanks.

@mwpenny
Copy link
Contributor Author

mwpenny commented Mar 20, 2023

@maziac From what I can tell, the block you mentioned handles cases where multiple labels refer to the same constant/data. For example:

; All labels will be have the same vscode.SymbolKind applied (SymbolKind.Field)
label1:
label2:
label3:
    defb 123

The code keeps track of all consecutive symbols in lastSymbols and then goes back and updates their kind if necessary once a constant or data definition is encountered.

@mwpenny mwpenny deleted the module-fix branch March 21, 2023 01:30
@maziac
Copy link
Owner

maziac commented Mar 22, 2023

Thanks.

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