Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Fix MultiInfo parser + serialization bug #2265

Merged
merged 7 commits into from
Jun 18, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 52 additions & 2 deletions src/main/scala/firrtl/Visitor.scala
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,56 @@ class Visitor(infoMode: InfoMode) extends AbstractParseTreeVisitor[FirrtlNode] w
private def string2Int(s: String): Int = string2BigInt(s).toInt

private def visitInfo(ctx: Option[InfoContext], parentCtx: ParserRuleContext): Info = {
// Convert a compressed FileInfo string into either into a singular FileInfo or a MultiInfo
// consisting of several FileInfos
def parseCompressedInfo(escaped: String): Info = {
var out: Seq[FileInfo] = Seq()

// Regular expression to match and capture the general File.format line:col pattern.
// Also matches the remaining part of the string which doesn't match the expression;
// which will be passed directly into the output as a FileInfo.
val splitCompressedInfo = """([^\s:]+)((?: \d+:(?:\d+|\{\d+(?:,\d+)+\}))+)|(?:[^\s].*)""".r

// Regular expression to capture the line number and column numbers in the compressed file info pattern.
val splitLineDescriptors = """(\d+):((?:\d+|\{\d+(?:,\d+)+\}))""".r

// Regular expression to match against individual column numbers in each line:col or line:{col1,col2}
// descriptor.
val splitColDescriptors = """\d+""".r

val matches = splitCompressedInfo.findAllIn(escaped)

// Grab each File.format line:col token from the input string
splitCompressedInfo.findAllIn(escaped).matchData.foreach { info =>
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary curly braces here

Option(info.group(1)) match {
// If there were no subgroups, the regex matched against a non-conforming source locator
// pattern, so do not process it
case None => out = out :+ ir.FileInfo.fromEscaped(info.toString)
case Some(file) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case Some(file) => {
case Some(file) =>

Again, unnecessary curly braces.

val lineDescriptors = info.group(2)
// Grab each line:col values from the separated (compressed) FileInfo.
splitLineDescriptors.findAllIn(lineDescriptors).matchData.foreach { lineDescriptor =>
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Again :)

val line = lineDescriptor.group(1)
val cols = lineDescriptor.group(2)
splitColDescriptors.findAllIn(cols).matchData.foreach {
// Use all the necessary info to generate normal uncompressed FileInfos
col => out = out :+ ir.FileInfo.fromEscaped(s"$file $line:$col")
}
}
}
}
}
}
}

out.size match {
case 0 => NoInfo
case 1 => out.head
case _ => new MultiInfo(out)
}
}
def genInfo(filename: String): String =
stripPath(filename) + " " + parentCtx.getStart.getLine + ":" +
parentCtx.getStart.getCharPositionInLine
Expand All @@ -68,11 +118,11 @@ class Visitor(infoMode: InfoMode) extends AbstractParseTreeVisitor[FirrtlNode] w
infoMode match {
case UseInfo =>
if (useInfo.length == 0) NoInfo
else ir.FileInfo.fromEscaped(useInfo)
else parseCompressedInfo(useInfo)
case AppendInfo(filename) if (useInfo.length == 0) =>
ir.FileInfo.fromEscaped(genInfo(filename))
case AppendInfo(filename) =>
val useFileInfo = ir.FileInfo.fromEscaped(useInfo)
val useFileInfo = parseCompressedInfo(useInfo)
val newFileInfo = ir.FileInfo.fromEscaped(genInfo(filename))
ir.MultiInfo(useFileInfo, newFileInfo)
case GenInfo(filename) =>
Expand Down
15 changes: 14 additions & 1 deletion src/test/scala/firrtlTests/InfoSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ class InfoSpec extends FirrtlFlatSpec with FirrtlMatchers {
" wire [31:0] a = c ? b : d; // @[A 1:{2,3,4} B 2:3 4:5]"
)
result("A 2:3", "B 1:{2,3,4}", "C 4:5") should containLine(
" wire [31:0] a = c ? b : d; // @[B 1:{2,3,4} A 2:3 C 4:5]"
" wire [31:0] a = c ? b : d; // @[A 2:3 B 1:{2,3,4} C 4:5]"
)
}

Expand Down Expand Up @@ -321,4 +321,17 @@ class InfoSpec extends FirrtlFlatSpec with FirrtlMatchers {
for (line <- check)
result should containLine(line)
}

"Source Locators" should "not loose information when going through serialization + parsing" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Source Locators" should "not loose information when going through serialization + parsing" in {
"Source Locators" should "not lose information when going through serialization + parsing" in {

Copy link
Contributor

Choose a reason for hiding this comment

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

my bad :(

def check(info: ir.Info): Unit = {
assert(Parser.parseInfo(info.serialize) == info)
}

check(ir.NoInfo)
check(ir.FileInfo("B"))
check(ir.FileInfo("A 4:5"))
check(ir.FileInfo("A 4:6"))
check(ir.MultiInfo(ir.FileInfo("A 4:5"), ir.FileInfo("B 5:5")))
check(ir.MultiInfo(ir.FileInfo("A 4:5"), ir.FileInfo("A 5:5")))
}
}