Skip to content

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

Open
waynewangyuxuan wants to merge 1 commit into
nyuoss:team-4-Implementationfrom
YuWei-CH:new-language-final-submission
Open

Team-4-New-Language-Final-Submission#29
waynewangyuxuan wants to merge 1 commit into
nyuoss:team-4-Implementationfrom
YuWei-CH:new-language-final-submission

Conversation

@waynewangyuxuan

Copy link
Copy Markdown

No description provided.

@yw5490

yw5490 commented Dec 12, 2024

Copy link
Copy Markdown

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

@marwanabdelhameed

Copy link
Copy Markdown

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