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

Jump to bottom

Verilog::Preproc misdocuments def_exists instead of def_params #1658

Closed
veripoolbot opened this issue Jan 9, 2020 · 4 comments
Closed

Verilog::Preproc misdocuments def_exists instead of def_params #1658

veripoolbot opened this issue Jan 9, 2020 · 4 comments
Assignees
@wsnyder

Comments

@veripoolbot
Copy link
Collaborator

Author Name: Topa Tota
Original Redmine Issue: 1658 from https://www.veripool.org

Original Assignee: Wilson Snyder (@wsnyder)

In the package Verilog::Preproc, the callback def_exists doesn't get called at all (at least I haven't found any situation where it gets called). I believe when Verilog:Preproc finds ifdef, ifndef, and `elsif, it should call def_exists. However, it is calling def_params instead. Below is an example:

1. test.pl
use strict;
use warnings;

package MyPreproc;
use Verilog::Preproc;
use parent qw(Verilog::Preproc);

sub new {
     my $class = shift;
     my $self = $class->SUPER::new(@_);
     bless $self, $class;
     return $self;
}

sub def_params {
     my $self = shift;
     print "def_params called!\n";
     return $self->SUPER::def_params(@_);
}

sub def_exists {
     my $self = shift;
     print "def_exists called!\n";
     return $self->SUPER::def_exists(@_);
}

package main;
use Verilog::Getopt;

my $opt = new Verilog::Getopt();

my $preproc = new MyPreproc();

$preproc->open("test.sv");

while (defined (my $line = $preproc->getline())) { }

</code>

1. test.sv
`define DEBUG 1

`ifdef DEBUG
`endif

</code>

The reason I think it should call def_exists is because in file VPreProc.cpp lines 829 and 845 (Module version: 3.470), m_preprocp->defExists is called when the state is ps_DEFNAME_IFDEF, ps_DEFNAME_IFNDEF, or ps_DEFNAME_ELSIF. I also think checking a token is defined should call both def_exists (VPreProc.cpp line 794) and def_params (VPreProc.cpp line 1186). It only calls def_params.

Also, def_params is an undocumented callback in the docs.
@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: Topa Tota
Original Date: 2020-01-09T02:55:35Z

Looking more through the code.

defExists calls defParams (Preproc.xs line 134) which calls def_params (Preproc.xs line 139). I think the fix is either

  • remove def_exists from docs and replace it with def_params

OR ideally

  • defExists should call def_exists (Preproc.xs line 134). Then def_exists should call def_params (in Preproc.pm) or better to have def_exists not call def_params at all (to give end-user more flexibility).

@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2020-01-09T10:48:06Z

Hi, thanks for your report.

I prefer to keep it backward compatible, so I've committed fixing this as a documentation change, removing def_exists and documenting def_params. Note this also matches up as def_params calls the option handler's defparam.

@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: Topa Tota
Original Date: 2020-01-10T04:55:42Z

Wilson Snyder wrote:

Hi, thanks for your report.

I prefer to keep it backward compatible, so I've committed fixing this as a documentation change, removing def_exists and documenting def_params. Note this also matches up as def_params calls the option handler's defparam.

Ok, thanks for the fix. I just noticed you made the changes. However, def_params is slightly different from def_exists. The docs should read like this:

Called to determine if the define exists and the parameters it expects.
Return undef if the define doesn't exist, 0 if the define exists with no arguments,
or argument list with leading parenthesis if the define has arguments. 
Defaults to use options object's defparams method.

@veripoolbot
Copy link
Collaborator Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2020-01-10T11:10:41Z

Thanks for the rewording, pushed to git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wsnyder wsnyder

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wsnyder @veripoolbot