dalance / sv-parser

SystemVerilog parser library fully compliant with IEEE 1800-2017
Other
383 stars 49 forks source link

Fix issues #49 and #50 #51

Closed erihsu closed 2 years ago

erihsu commented 2 years ago

Hi, dalance Thanks for your great work on sv-parser, nice project. The pull request is trying to two issues opened by me. Changes are quite minor and these are just to support svfmt

  1. sv-parser now can parse class definition now by add variant in Description enum, fix #50
  2. sv-parser now can differentiate newline("\t") and space("\t" "\s"), fix #49
dalance commented 2 years ago

Thank you for your contribution. "2." seems to be good.

In SystemVerilog LRM, class_declaration is not allowd on the root of source code.

source_text ::= [ timeunits_declaration ] { description }
description ::=
module_declaration
| udp_declaration
| interface_declaration
| program_declaration
| package_declaration
| { attribute_instance } package_item
| { attribute_instance } bind_directive
| config_declaration

So the addtion of class_declaration seems to exceed the language standard. How about it?

erihsu commented 2 years ago

I agree with

So the addtion of class_declaration seems to exceed the language standard

As I have never fully read SystemVerilog LRM before, it's quite reasonable if LRM states the Description definition. Maybe I would later consider add formatting strategy without changing parser rule.

erihsu commented 2 years ago

Should I do something to trigger pull request merge ? I notice it's pended for nearly a month😂@dalance

dalance commented 2 years ago

I want to merge the change about whitespace. Could you remove the addition of class_declaration?

erihsu commented 2 years ago

Hi dalance, I have reverted changes in class declaration. Before that, I have thought you can personally pick partial commit from the pull request, never mind.

dalance commented 2 years ago

Thanks!