Refactor ISQL creating FrontendParser class #8358
Conversation
asfernandes
force-pushed
the
work/isql-frontend-parser
branch
2 times, most recently
from
9dfa495 to
4894def
Compare
asfernandes
force-pushed
the
work/isql-frontend-parser
branch
from
4894def to
7357355
Compare
TreeHunter9
left a comment
TreeHunter9
left a comment
There was a problem hiding this comment.
Sorry for the late review to an already merged PR.
| std::string rawText; | ||
| std::string processedText; | ||
|
|
||
| std::string getProcessedString() const |
There was a problem hiding this comment.
Maybe better return const std::string&, NRVO or move constructor won't apply here, we will always make a copy, and as I can see in code, we use this function for simple read.
| (!first && c >= '0' && c <= '9') || | ||
| (!first && c == '$') || | ||
| (!first && c == '_'))) |
There was a problem hiding this comment.
Maybe better would be
(!first && ((c >= '0' && c <= '9') ||
(c == '$') ||
(c == '_')))
So we don't have to check first every time
|
|
||
| const auto commandToken = lexer.getToken(); | ||
|
|
||
| if (commandToken.type == Token::TYPE_OTHER) |
There was a problem hiding this comment.
Maybe invert this if statement to reduce nesting?
if (commandToken.type != Token::TYPE_OTHER)
return InvalidNode();
| const auto objectTypeText = std::string(objectType->c_str()); | ||
|
|
||
| if ((objectTypeText.length() >= 7 && std::string(TOKEN_COLLATES).find(objectTypeText) == 0) || | ||
| (objectTypeText.length() >= 9 && std::string(TOKEN_COLLATIONS).find(objectTypeText) == 0)) | ||
| { | ||
| node.objType = obj_collation; | ||
| } | ||
| else if (objectTypeText.length() >= 4 && std::string(TOKEN_FUNCTIONS).find(objectTypeText) == 0) | ||
| node.objType = obj_udf; | ||
| else if (objectTypeText.length() >= 5 && std::string(TOKEN_TABLES).find(objectTypeText) == 0) | ||
| node.objType = obj_relation; | ||
| else if (objectTypeText.length() >= 4 && std::string(TOKEN_ROLES).find(objectTypeText) == 0) | ||
| node.objType = obj_sql_role; | ||
| else if (objectTypeText.length() >= 4 && std::string(TOKEN_PROCEDURES).find(objectTypeText) == 0) | ||
| node.objType = obj_procedure; | ||
| else if (objectTypeText.length() >= 4 && std::string(TOKEN_PACKAGES).find(objectTypeText) == 0) | ||
| node.objType = obj_package_header; | ||
| else if (objectTypeText.length() >= 3 && std::string(TOKEN_PUBLICATIONS).find(objectTypeText) == 0) |
There was a problem hiding this comment.
Why are you using std::string here? std::string_view has find() too.
| if (const auto tableName = parseName()) | ||
| { | ||
| AddNode node; | ||
| node.tableName = std::move(tableName.value()); |
There was a problem hiding this comment.
IIRC std::move() on const object will return const T&& which needs to be handled with different approach and default move ctor or assignment operator will not kick in because they expect T&&. You have a lot of std::move with const object, so I guess it's better to test it, maybe I'm wrong.
| if (parseEof()) | ||
| return ShowUsersNode(); | ||
| } | ||
| else if (text == TOKEN_VER || text == TOKEN_VERSION) |
There was a problem hiding this comment.
It looks a bit inconsistent, previousif()'s used different approach to calculate short version.
There was a problem hiding this comment.
This command used to be different, only VER or VERSION was recognized.
| { | ||
| node.objType = obj_collation; | ||
| } | ||
| else if (objectTypeText.length() >= 4 && std::string(TOKEN_FUNCTIONS).find(objectTypeText) == 0) |
There was a problem hiding this comment.
Maybe move objectTypeText.length() >= 4 && std::string(TOKEN_FUNCTIONS).find(objectTypeText) == 0 to a separate function, this pattern occurs frequently?
| if (first) | ||
| return std::nullopt; | ||
|
|
||
| return FrontendLexer::trim(std::string(startPos, lastPos)); |
There was a problem hiding this comment.
I'm got a little bit confused when I saw std::string(startPos, lastPos), maybe add suffix It for iterators?
| FrontendParser(std::string_view statement, const Options& aOptions) | ||
| : lexer(statement), | ||
| options(aOptions) | ||
| { | ||
| } |
There was a problem hiding this comment.
It looks like it should be private, no?
3 participants
This is a work preparing ISQL for schemas support which I prefer to be done first and separated from the schemas branch.