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

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

Comments

@veripoolbot
Copy link
Collaborator

@veripoolbot veripoolbot commented Jan 9, 2020


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

This comment has been minimized.

Copy link
Collaborator Author

@veripoolbot veripoolbot commented Jan 9, 2020


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

This comment has been minimized.

Copy link
Collaborator Author

@veripoolbot veripoolbot commented Jan 9, 2020


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 veripoolbot closed this Jan 9, 2020
@veripoolbot

This comment has been minimized.

Copy link
Collaborator Author

@veripoolbot veripoolbot commented Jan 10, 2020


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

This comment has been minimized.

Copy link
Collaborator Author

@veripoolbot veripoolbot commented Jan 10, 2020


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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.