Skip to content
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

Team-4-New-Language-Final-Submission #29

Open
wants to merge 1 commit into
base: team-4-Implementation
Choose a base branch
from

Conversation

waynewangyuxuan
Copy link

No description provided.

@yw5490
Copy link

yw5490 commented Dec 12, 2024

This PR is to introduce a new language written in C++ that leverages the g++ compiler for ninja manifest compilation, anticipated to be significantly faster than parsing through ninja manifest_parser. This involves the creation of a dedicated header file defining necessary classes alongside an example C++ file, manifest.cc, which users are expected to adapt.

Review Sequence:

  1. Design Document: Start with the design document to understand the high-level architecture and the rationale behind the new language design.
  2. manifest.h: Next, review the manifest.h file to familiarize yourself with the new class definitions and interfaces.
  3. manifest.cc: Finally, examine the manifest.cc file to see an example of how users define build processes using the new language syntax.

Test Plan:
Added the following test to be automatically performed by Circle CI:

check_success() {
  if [ $? -eq 0 ]; then
    echo "$1"
  else
    echo "$1"
    exit 1
  fi
}

cd src/shadowdash

g++ -w -std=c++17 -fPIC -c manifest.cc  manifest.h
check_success ".o compiled successfully"

g++ -w -shared -o manifest.so manifest.o
check_success ".so compiled successfully"

cd ../..

Test Result:
Overall Test
Test to compile manifest.o and manifest.so

@MarwanWalid2
Copy link

Code review:
Code Functionality
Core Implementation
The implementation provides a comprehensive DSL for Ninja build configurations with strong type safety
The manifest system effectively handles variables, pools, rules, and builds through a well-structured C++ interface

Areas for Enhancement
No configuration for build output directory control
Missing platform-specific command handling for cross-platform compatibility
Pool depth validation could be more robust in the pool_ class implementation

Readability & Structure
Class Design
Inconsistent naming conventions with underscore suffixes (pool_, default_) compared to other class names (Token, build, rule)
Good use of modern C++ features like constexpr and initializer lists

Code Organization
Clear separation between interface (manifest.h) and implementation (manifest.cc)
Well-structured class hierarchies for build system components
Example usage in manifest.cc helps demonstrate the API

Standards Compatibility
Access Control
Token class exposes public fields that breaks encapsulation
Most other classes properly encapsulate their members

Error Handling
Basic exception handling implemented for pool configuration
Could benefit from additional error handling for:
Invalid build configurations
Missing dependencies
Malformed rule definitions

Testing & CI Integration
CircleCI configuration properly tests compilation of .o and .so files1
Build system integration tests are included
.gitignore appropriately configured for the generated file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants