Feature/semantic #253
Feature/semantic #253
Conversation
Dockerfile
Outdated
| @@ -16,6 +16,23 @@ RUN if [ "$DEBUG" = "true" ]; then apt-get update \ | |||
| && apt-get install --no-install-recommends -y gdb gdbserver \ | |||
| && apt-get clean; fi | |||
|
|
|||
| WORKDIR "${HOME}" | |||
| RUN mkdir antlr | |||
There was a problem hiding this comment.
Currently, variable HOME is pointing to /home/ubuntu. This antlr folder should get created inside the following path. Not at HOME.
/home/ubuntu/software
There was a problem hiding this comment.
okay sir, I'll put into this location, "/home/ubuntu/software". but after moving the generated files in to JasmineGraph, we don't need this folder anymore then this home/ubuntu/antlr folder get cleared.
| frontend_logger.log("AST is succssusfully analyzed", "log"); | ||
| }else | ||
| { | ||
| frontend_logger.error("query isn't semantically correct"); |
There was a problem hiding this comment.
Lets add the query also as part of the error message. This will give more details and support for debugging.
| #include "ASTLeafValue.h" | ||
| #include "/home/ubuntu/software/jasminegraph/code_generated/antlr/CypherBaseVisitor.h" | ||
|
|
||
| any visitOC_Cypher(CypherParser::OC_CypherContext *ctx) ; |
There was a problem hiding this comment.
Can we reorder the appearance of the function definitions in alphabetical order of the letters appearing after _. For example visitOC_Cypher we should use the letter C and the y if needed for alphabetical sorting.
There was a problem hiding this comment.
the order is very similar to hierarchy of the cypher grammar rule set. and this order was defined by antlr4 then it can be helpful for debugging. if it is necessary, I can re-order it.
There was a problem hiding this comment.
In this case lets not re-order the functions.
|
|
||
|
|
||
|
|
||
| any ASTBuilder::visitOC_Cypher(CypherParser::OC_CypherContext *ctx) { |
There was a problem hiding this comment.
Lets arrange these function declarations also based on the order of the function definitions appearance.
There was a problem hiding this comment.
the order is very similar to hierarchy of the cypher grammar rule set. and this order was defined by antlr4 then it can be helpful for debugging. if it is necessary, I can re-arrange it.
There was a problem hiding this comment.
In this case lets not re-order the functions.
| { | ||
| return false; | ||
| } | ||
| }else if(root->nodeType == "VARIABLE") |
There was a problem hiding this comment.
Can we move these literals to some constants and refer them here? I see same literal repeated several times here.
There was a problem hiding this comment.
sir, is it okay to move the constants to the const.cpp file in the util folder ?
There was a problem hiding this comment.
Yes, Its ok the way you have set the util folder currently.
| }else if (root->nodeType == "AS") | ||
| { | ||
| string ntype; | ||
| if (root->elements[0]->nodeType == "LIST" || root->elements[0]->nodeType == "LIST_COMPREHENSION") |
There was a problem hiding this comment.
Can there be scenario where root->elements becoming null?
There was a problem hiding this comment.
sir, it cannot be null, but it can be empty.
|
|
||
| // Report errors | ||
| void SemanticAnalyzer::reportError(const std::string &message, ASTNode* node) { | ||
| std::cerr << "Error: " << message << std::endl; |
There was a problem hiding this comment.
Ok, but there are some further changes required. Please check my new comments.
CMakeLists.txt
Outdated
| ) | ||
|
|
||
| if (CMAKE_BUILD_TYPE STREQUAL "DEBUG") | ||
| add_compile_options(-DUNIT_TEST) | ||
| endif () | ||
|
|
||
| add_library(JasmineGraphLib ${HEADERS} ${SOURCES}) | ||
| file(GLOB GENERATED_SRC ${CMAKE_SOURCE_DIR}/code_generated/antlr/*.cpp) |
There was a problem hiding this comment.
Where we will have the code_generated folder in the directory structure?
There was a problem hiding this comment.
home/ubuntu/software/jasminegraph/code_generated/antlr/*.cpp
this is the path to the code_generated folder
Dockerfile
Outdated
| @@ -1,4 +1,4 @@ | |||
| FROM miyurud/jasminegraph-prerequisites:20240101T095619 | |||
| FROM pre:latest | |||
There was a problem hiding this comment.
Why do we change the base image here?
There was a problem hiding this comment.
it was mistakenly commited my locally used prerequisite image. now I edited it and push it.
|
add semantic analyzer