Skip to content

Conversation

@Scheremo
Copy link
Contributor

@Scheremo Scheremo commented Oct 2, 2025

  • Use Slang buffers as source of truth

    • Replace llvm::SourceMgr-centric code with direct use of slang::SourceManager.
    • Add slang::BufferID mainBufferId and expose getMainBufferID().
  • Interval map now indexes Slang buffer pointers

    • New typedef: using SlangBufferPointer = char const *;.
    • Change from IntervalMap<const char*, …> to IntervalMap<SlangBufferPointer, …> (half-open).
    • Lookups are computed from slang::SourceManager::getSourceText(buf).data() + offset.
  • Remove LLVM SourceMgr plumbing

    • Drop members/APIs: sourceMgr, bufferIDMap, getSMLoc(...).
    • Update filePathMap to store (slang::BufferID, path) pairs.
    • getOrOpenFile(...) now assigns text via Slang (driver.sourceManager.assignText(...)) and caches the returned BufferID.
  • Implement LSP UTF-16 to Slang UTF-8 conversion

    • Add decodeUtf8(...) and lspPositionToOffset(...) helpers.
    • Use these helpers in getLocationsOf(...) and findReferencesOf(...) before interval lookups.
    • Clamp positions past end-of-line to buffer end; avoid OOB pointer math.
  • Definition & reference resolution updated

    • getLspLocation(slang::SourceLocation/Range) now uses Slang line/column and compares loc.buffer() directly to mainBufferId.
    • findReferencesOf(...) / getLocationsOf(...) locate the interval by pointer into the main buffer.
  • Top module discovery via syntax trees

    • Derive driver.options.topModules from CompilationUnitSyntax members by reading ModuleDeclarationSyntax headers (covers modules and packages).
    • Avoid creating a separate Compilation just to enumerate tops.

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

This looks fantastic, thank you for unifying them!

@Scheremo
Copy link
Contributor Author

Scheremo commented Oct 3, 2025

This looks fantastic, thank you for unifying them!

Thanks! I'll open this for review / merging now, do let me know if you have any points that stick out to you 😄
I'm happy with the current state, so feel free to merge if you don't find any issues.

@Scheremo Scheremo marked this pull request as ready for review October 3, 2025 08:08
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for improving buffer handling!

- **Use Slang buffers as source of truth**
  - Replace `llvm::SourceMgr`-centric code with direct use of `slang::SourceManager`.
  - Add `slang::BufferID mainBufferId` and expose `getMainBufferID()`.

- **Interval map now indexes Slang buffer pointers**
  - New typedef: `using SlangBufferPointer = char const *;`.
  - Change from `IntervalMap<const char*, …>` to `IntervalMap<SlangBufferPointer, …>` (half-open).
  - Lookups are computed from `slang::SourceManager::getSourceText(buf).data() + offset`.

- **Remove LLVM `SourceMgr` plumbing**
  - Drop members/APIs: `sourceMgr`, `bufferIDMap`, `getSMLoc(...)`.
  - Update `filePathMap` to store `(slang::BufferID, path)` pairs.
  - `getOrOpenFile(...)` now assigns text via Slang (`driver.sourceManager.assignText(...)`) and caches the returned `BufferID`.

- **Implement LSP UTF-16 to Slang UTF-8 conversion**
  - Add `decodeUtf8(...)` and `lspPositionToOffset(...)` helpers.
  - Use these helpers in `getLocationsOf(...)` and `findReferencesOf(...)` before interval lookups.
  - Clamp positions past end-of-line to buffer end; avoid OOB pointer math.

- **Definition & reference resolution updated**
  - `getLspLocation(slang::SourceLocation/Range)` now uses Slang line/column and compares `loc.buffer()` directly to `mainBufferId`.
  - `findReferencesOf(...)` / `getLocationsOf(...)` locate the interval by pointer into the **main** buffer.

- **Top module discovery via syntax trees**
  - Derive `driver.options.topModules` from `CompilationUnitSyntax` members by reading `ModuleDeclarationSyntax` headers (covers modules and packages).
  - Avoid creating a separate `Compilation` just to enumerate tops.
@Scheremo
Copy link
Contributor Author

Scheremo commented Oct 4, 2025

LGTM, thank you for improving buffer handling!

Thank you once more for the helpful and targetted feedback. You really helped to point out some areas to improve further!
I'm okay with merging at this point, but would be interested in your opinion on this:

I think unit tests make a lot of sense (easier to test things that way), but I would suggest to first break out VerilogServer, VerilogServerContext, VerilogTextFile and VerilogDocument into separate .cc files with corresponding header files. We could then add a unittest directory in the VerilogServerImpl directory, e.g. like this:

VerilogServerImpl/
├─ CMakeLists.txt
├─ include/
│ ├─ VerilogTextFile.h
│ ├─ VerilogDocument.h
│ ├─ VerilogServerContext.h
│ └─ VerilogServer.h
├─ src/
│ ├─ VerilogTextFile.cc
│ ├─ VerilogDocument.cc
│ ├─ VerilogServerContext.cc
│ └─ VerilogServer.cc
└─ unittests/
└─ VerilogDocumentTests.cpp

If something like this is fine for you, I can open a separate PR to implement it. Let me know what you think!

@uenoku
Copy link
Member

uenoku commented Oct 4, 2025

I'm okay with merging at this point, but would be interested in your opinion on this:

Thanks! The current PR looks also great me!

with corresponding header files. We could then add a unittest directory in the VerilogServerImpl directory, e.g. like this:

I agree, I think this looks awesome! I also think separating headers is great. One nitpick is we probably want to use flat structures without include and src directories. LLVM somehow prefers this kind of flat directories for header files used under lib/ directories.

VerilogServerImpl/
|- CMakeLists.txt
|- VerilogTextFile.h
|- VerilogTextFile.cpp
|- VerilogDocument.h
|- VerilogDocument.cpp
...

@uenoku uenoku merged commit 35af03c into llvm:main Oct 4, 2025
7 checks passed
@Scheremo Scheremo deleted the pr-slang-project branch October 21, 2025 07:49
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