-
Notifications
You must be signed in to change notification settings - Fork 44
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
Improve links between individual constructs in internal representation (issue #73) #96
Conversation
…mport of symbol.h from literal.h
…inition so renaming will be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me. There are just 2 things I have commented on but they are not that important so that this PR is blocked by them so I'll merge this and please take a look at those comment and just answer/incorporate them later. Thanks.
namespace yaramod { | ||
|
||
///< Type of the expression. | ||
enum class ExpressionType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the motivation for taking out this type out of Expression
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Literal
class now needs to access Symbol::getName
method. That means it needs to include the symbol.h file. In the symbol.h
file there has been include of expression.h
so we would end up with cyclic dependency (see picture). Luckily, the Symbol
class only needs access to ExpressionType
which has been extracted from Expression::Type
to avoid this cyclic dependency.
else if (literal.isSymbol()) | ||
os << literal.getSymbol()->getName(); | ||
else if (literal.isLiteralReference()) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: there are no curly braces in other parts of this condition so I wouldn't expect them here either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix it in my next PR, thank you for merging this.
In this PR we allow
yaramod::Literal
to reference another of it's kind so we can create links between tokens in a tokenstream. This brings a possibility to rename strings and rules easily. We had to moveyaramod::Token::Type
to a new file and make it a classyaramod::TokenType
and similar withyaramod::Expression::Type
.