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

Rejects ifdef'd module port declaration line beginning with ',' #267

Open
JakubJatczak opened this issue Apr 14, 2020 · 4 comments
Open
Labels
preprocessor anything related to preprocessing (conditionals, macros, etc.) rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017).

Comments

@JakubJatczak
Copy link

Running verilog_format with this reduced example:

module mod_name (input bus_a, output bus_b
`ifdef USE_PG_PIN
    , input pg_pin
`endif
    );
endmodule 

fails with:

1.v:3:16: syntax error, rejected "," (https://github.com/google/verible).
1.v:6:1: syntax error, rejected "endmodule" (https://github.com/google/verible).
@fangism
Copy link
Collaborator

fangism commented Apr 14, 2020

Yes, this is a known limitation. Verible only reads un-preprocessed files without proper preprocessing, and the grammar as implemented has limited support for where `ifdefss may lie.

The formatter is also limited to working on files it can parse, subject to the parser's limitations.

Issue #228 is in the same class of issues, and mentions potential strategies in the future, such as composite parsing/formatting.

@fangism fangism added preprocessor anything related to preprocessing (conditionals, macros, etc.) rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017). labels Apr 14, 2020
@fangism
Copy link
Collaborator

fangism commented Apr 14, 2020

I understand you may or may not have the liberty to rewrite the above example, but:

module mod_name (input bus_a,
`ifdef USE_PG_PIN
    input pg_pin,
`endif
    output bus_b
    );
endmodule 

should be acceptable to Verible's parser/formatter.

@fangism fangism changed the title Syntax error on lines beggining with ',' Rejects ifdef'd module port declaration line beginning with ',' Apr 14, 2020
@fangism
Copy link
Collaborator

fangism commented Apr 15, 2020

@pmaupin I think you meant this one is a dupe of #241 (but you mentioned in #268 ).

@pmaupin
Copy link

pmaupin commented Apr 15, 2020

Yes, fat-fingered URL on my tablet. Removed extraneous comment in issue 268. Thanks!

nikhiljha pushed a commit to nikhiljha/verible that referenced this issue Sep 27, 2022
kbieganski added a commit to antmicro/verible that referenced this issue Apr 28, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress. Hoping to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue Apr 28, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 24, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preprocessor anything related to preprocessing (conditionals, macros, etc.) rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017).
Projects
None yet
Development

No branches or pull requests

3 participants