verilog/analysis: Add ParsedVerilogSourceFile class #745
Conversation
google-cla
bot
added
the
cla: yes
ghost
force-pushed
the
parsed-verilog-source-file
branch
from
b798442 to
47e9bf0
Compare
Signed-off-by: Lukasz Dalek <[email protected]>
There was a problem hiding this comment.
(This looks like a preparatory change for some other change. What are you going to use it for ?)
Looks mostly good, but I think using a const reference in the constructor and holding on to it might need either some specific explanation or, better, passing a pointer to keep the reader on the right track.
verilog/analysis/verilog_project.h
Outdated
| public: | ||
| // filename can be fake, it is not used to open any file. | ||
| ParsedVerilogSourceFile(absl::string_view filename, | ||
| const verible::TextStructureView& text_structure, |
There was a problem hiding this comment.
the ownership of this probably needs to be described in the constructor description, as this expects the text_structure be available for the lifetime of this object.
I'd suggest to make this a const verible::TextStructuredView*, also text_structure_. Passing a pointer makes it more obvious that the object is keeping a reference to it.
(usually, when we pass const references in constructors, a copy is made, so this is what the programmer expects. If we keep a reference to the thing we can make it more visible by passing a pointer)
| text_structure_(text_structure) {} | ||
|
|
||
| // Do nothing (file contents already loaded) | ||
| absl::Status Open() override; |
There was a problem hiding this comment.
Given that we don't expect to derive further, we can make the override a final (same below; same in InMemoryVerilogSourceFile). Might need a class ParsedVerilogSourceFile final : ... above.
There was a problem hiding this comment.
I made the whole class final (same with InMemoryVerilogSourceFile class).
There was a problem hiding this comment.
Nice. At that point, the compiler knows that once it can proof that there can only be a ParsedVerilogSourceFile in a particular instance, it can replace calls to the virtual methods with direkt calls and does not have to go through the virtual method table. While here the performance win is probably not noticeable, it is a win for the reader of the code.
So for readability, I'd then also suggest to replace override with final in all the methods so that it is easier to see for the reader at first glance without having looked at the class declaration above.
Not critical for this review, can be changed later.
This class is an extended version of InMemoryVerilogSourceFile. It wraps a TextStructureView object and allows to skip loading and parsing phase. This class may be used in e.g. TextStructureLinterRules. Signed-off-by: Lukasz Dalek <[email protected]>
InMemoryVerilogSourceFile class is implemented for testing purposes. Marking it final as we don't expect to derive it further. Signed-off-by: Lukasz Dalek <[email protected]>
ghost
force-pushed
the
parsed-verilog-source-file
branch
from
47e9bf0 to
82d56e3
Compare
…ipsalliance#746 Signed-off-by: Lukasz Dalek <[email protected]>
This class is an extended version of InMemoryVerilogSourceFile.
It wraps a TextStructureView object and allows to skip loading and
parsing phase.