Converter #17
Converter #17
Conversation
change clang test name update clangtest file
Co-authored-by: deflucaseng <[email protected]>
|
Commits should be segmented based into appropriate sized commits - currently, everything seems to be in one huge commit. |
| - `main.py`: Orchestrate the compilation pipeline | ||
|
|
||
| ## To Run | ||
|
|
There was a problem hiding this comment.
Could add build instructions for converter here.
| ## To Run | ||
|
|
||
| ```bash | ||
| ./build/ninja_converter input.ninja output.cpp |
There was a problem hiding this comment.
Usage instructions need correction. Only 1 argument is required by the program.
There was a problem hiding this comment.
I wasn't able to locate the manifest.h header file. If you are using the version from the professor's notes, I've noticed that the build function in the generated build.ninja.cc has 8 arguments, while the corresponding build function in manifest.h only has 7 arguments. If you can add manifest.h header file, that would be good.
There was a problem hiding this comment.
Unit tests performed against lexer and parser related functions.
Overall test was also performed and build.ninja.cc can be successfully generated.
| std::vector<Token> Lexer::scanTokens() { | ||
| while (!isAtEnd()) { | ||
| Token token = nextToken(); | ||
| std::cout << "Scanned token: type=" << static_cast<int>(token.type()) << " lexeme=" << token.lexeme() << std::endl; // Debugging output |
There was a problem hiding this comment.
Debug output (lexer and parser) should be disabled by default (shouldn't go to stdout atleast, maybe send it to a debug file).
Currently it is overflowing the screen.
There was a problem hiding this comment.
our development is still work in progress but for release version, we will take that in mind. thank you
There was a problem hiding this comment.
Granularity:
The files generally have appropriate granularity, though a few functions in parser.cpp and build_elements.cpp could be further decomposed.
• For example, parseRule() could have helper functions to parse the command and variables separately.
• And also for expandCommand() function in build_elements.cpp, it can be breakdown a little possibly with a helper function which focuses on isolating variable lookup, making expandCommand() more readable.
Functionality:
The code appears to function as intended; however scalability can be improved:
main.cpp:
The primary purpose of main.cpp is to read a Ninja build file and refactor its content into C++ code in a format that can be compiled (e.g., ninja.build.cc).
This involves parsing the Ninja build file syntax, processing its elements (variables, rules, build targets), and converting them to C++ code that defines these elements in a form that a C++ compiler can later interpret and use.
Supporting Components:
lexer.cpp: This component tokenizes the raw text of the build file into meaningful units (tokens) like keywords (rule, build, default), variables, and special syntax symbols (=, :). Tokenizing makes it easier for the parser to work with structured data.
token.cpp: This file defines the Token class and related functions. token.cpp encapsulates the properties of tokens (like type, value, and position in the file), making it easier to interpret different parts of the build file.
parser.cpp: The parser uses tokens from lexer.cpp to create higher-level constructs (abstract syntax tree, or AST) representing the structure of the Ninja build file. It interprets sequences of tokens to identify variables, rules, build targets, and defaults, organizing these into a form that main.cpp can further process.
build_elements.cpp: This component contains classes like Variable, Rule, BuildTarget, and BuildFile, which represent the core elements in a Ninja build file. It provides functions for formatting these elements in a C++-compatible way (e.g., toCodeString or toString).
Scalability:
The code can generate build.ninja.cc file on build.ninja example for hello.cc. However seems still needs more work to be able to convert from CMake generated build.ninja file (for Zlib for LLVM).
Readability:
Overall, readability is good, supporting functions are well organized.
Standards Compliance:
Good in general.
Testing:
Unit tests performed against lexer and parser related functions.
Overall test was also performed and build.ninja.cc can be successfully generated for simple build.ninja file (example build file provided for hello.cc). However it seems needs more work to be able to convert from CMake generated build.ninja file for larger build project like (Zlib or LLVM).
| ## To Run | ||
|
|
||
| ```bash | ||
| ./build/ninja_converter input.ninja output.cpp |
There was a problem hiding this comment.
I wasn't able to locate the manifest.h header file. If you are using the version from the professor's notes, I've noticed that the build function in the generated build.ninja.cc has 8 arguments, while the corresponding build function in manifest.h only has 7 arguments. If you can add manifest.h header file, that would be good.
There was a problem hiding this comment.
The current implementation of expandCommand() is functioning as intended. However it could benefit from breaking down its logic further, potentially by introducing a helper function dedicated to handling variable lookups. This would improve readability by isolating specific responsibilities within the function.
| ## To Run | ||
|
|
||
| ```bash | ||
| ./build/ninja_converter input.ninja output.cpp |
There was a problem hiding this comment.
Unit tests performed against lexer and parser related functions.
Overall test was also performed and build.ninja.cc can be successfully generated.
There was a problem hiding this comment.
This suffers from some of the common problems I have noticed before.
- How is each file in your PR connected to the task at hand?
- You should not have files that are irrelevant to the task.
- Make sure files that should be .gitignore are in .gitignore
- E.g. .DS_Store
- Make sure secrets are not checked in!
- Make sure there is a clear and separated API!
Not all of these apply to you but some do.
Please address them and let me know and I will look again.
|
we are continuing working on new PR #25 @kyotov please check check updated |
now compiling and converting