Skip to content

Conversation

@adkian-sifive
Copy link
Contributor

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

In Scala 3, stringification of C++ arguments in the svsim Verilator backend was leading to the arguments being wrapped in brackets instead of a space-joined string. This change updates the argument generation to stringify arguments individually before joining them, fixing the problem in Scala 3 testing

Type of Improvement

  • Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

Svsim Verilator Backend argument generation was refactored to work with Scala 3

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@adkian-sifive adkian-sifive added the Scala 3 Changes related to upgrading to Scala 3 label Oct 2, 2025
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

LGTM

This entire file is very messy at this point. This isn't the fault of this PR, though. We can clean this up after Scala 3 gets turned on.

Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

I don't have a strong objection to this, but I would like a more detailed explanation for what exactly was different about Scala 3 here. Can you provide a simple example of the different? Are we sure this can't be fixed with a simpler change to toCommandlineArgument?

@adkian-sifive
Copy link
Contributor Author

Issue is that in Scala 3 a Seq[Seq[String]] flattened and stringified as

@main def main =
  val cflags: Seq[Seq[String]] = Seq(
    Seq("-std=c++17"),
    Seq("-I$(shell pwd)"),
    Seq("-DSVSIM_ENABLE_VERILATOR_SUPPORT")
  )

  val buggy = cflags.flatten.mkString(" ")
  println(buggy)

prints [-std=c++17] [-I$(shell, pwd)] [-DSVSIM_ENABLE_VERILATOR_SUPPORT], while in Scala 2 it's space-separated as -std=c++17 -I$(shell pwd) -DSVSIM_ENABLE_VERILATOR_SUPPORT.

@adkian-sifive adkian-sifive merged commit 5121534 into main Oct 6, 2025
19 checks passed
@adkian-sifive adkian-sifive deleted the scala3-svsim-backend-string-fixes branch October 6, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scala 3 Changes related to upgrading to Scala 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants