Joshua Boyd wrote:
GCC doesn't seem care about my findings, but other compilers do care. I
am attempting to use SunStudio instead of GCC to build this. It has
been my experience that making code build with as many compilers as
possible tends to result in better code. For the sake of time, I'm
ignoring warnings.
All this I agree with. Thank you.
For instance, the file Statement.h. You have the PNoop class, which
implements the Statement class. The Statement class is an abstract
class. PNoop is supposed to be an implementation of that class, but
because it doesn't implement a destructor, it is itself also an abstract
class.
An empty destructor would be correct, yes. It can be in-line, or it
can be external. Inline is convenient and efficent in this case, I think.
The next place I see the same sort of error is in vpi_callback.cc, where
sync_cb also doesn't implement a destructor. I think that anyplace a
parent class has an abstract destructor, and you have a child class that
doesn't implement a destructor, that it is probably safe to stick a
dummy destructor as described above. Thus, is vpi_callback.cc, after
line 83, I added the line ~sync_cb () { }. Of course, if sync_cb was
ever to be a parent class, then perhaps it should be virtual ~sync_cb ()
{ } instead, but it doesn't appear that that is ever the case.
Once a method, including destructor, is made virtual, it remains
virtual all the way down the hierarchy. The "virtual" keword is not
needed in all the derived classes, although it is sometimes good to
put it there for documentation purposes.
As a rule, a missing virtual destructor is the same as an empty virtual
destructor.
Next in the file vpi_priv.cc, on lines 312 and 421, you give an integer
as a constant in a call to pow. As you want the result in a double, it
might be best to coerce the constant to a double so the compiler is
absolutely clear on which std::pow function to use.
I think changing 10 to 10.0 in that case would do the trick. I was
going to say that everything is integer there, but when dealing with
pico-'s and femto-'s, the range may be such that you really do want
a real value instead of an integer, for its range.
Also, in that same file, vpip_put_value_event has the same abstract
class problem as sync_cb in vpi_callback.cc. I again fixed it with a
~vpip_put_value_event() { } after line 397 in the same file.
vpi_time.cc, line 181 has the same problem with std::pow.
The other problems I've found so far have been configure problems, but
while I can fix them in the resulting Makefiles, I still have no clue
how to fix the configure script.
However, I am happy to say that this time, when I got the software
built, it worked (the last time I made it through the process, iverilog
would compile input files, but running the resulting executables would
spew link errors), at least for some super simple examples.
Well it is good that it works that far. I'm doing an FPGA design with
the CVS version, so it should be working pretty well. Solaris shouldn't
be a problem (even though it sometimes seems to be).
These all sound reasonable to me, so by all means send patches and I'll
merge them in. And by all means file bug reports for things that do not
work but you think should.
Thanks,
--
Steve Williams "The woods are lovely, dark and deep.
steve at icarus.com But I have promises to keep,
http://www.icarus.com and lines to code before I sleep,
http://www.picturel.com And lines to code before I sleep."
