Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Team2 newlang design #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mayank-ramnani
Copy link

Core Components

We are using the string_view for memory efficiency like in the Expression class, representing three types of command components

Our Build class uses vectors to maintain input/output relationships and order

We implemented thread-safe job management (POOL) using atomic counters

Data Structures
unordered_map for rules/variables (O(1) lookup)
vector for builds (preserves order)
string_view throughout to minimize copying
optional for rule attributes

Build Process

The system recursively checks dependencies using file timestamps and handles missing files and circular dependencies. We used atomic operations for thread safety during parallel builds.

Testing

Currently, this was tested on a small subset of files in zlib (not setting up BUILD for all targets just for testing). Will wait until we get a full converted main.cc that was converted from a build.ninja that matches our language to test fully.

@dl5214
Copy link

dl5214 commented Dec 4, 2024

Code Review by

Jasmine Wu (@jazwu)
Shihui Huang (@Shihuihuang1103)
Dong Li (@dl5214)


Overall Comments

This PR introduces a critical new feature for the redesigned ShadowDash language. While the implementation is robust and comprehensive, improvements in structure and documentation can enhance clarity and ease of review.


Feedback

1. Change Granularity

  • The PR includes one commit, adhering to PR etiquette.
  • It introduces a new feature with changes to main.cpp and manifest.h.
  • However, modifications related to CI/CD configuration should ideally be in a separate PR to maintain focus and clarity. The CI/CD pipeline file should be introduced prior to this PR. If this PR introduces new features requiring updates to the pipeline, those changes can be included as part of this PR’s diff.

2. Correctness & Functionality

  • The new language design correctly incorporates Ninja syntax elements such as BUILD, DEFAULT, PHONY, and POOL.
  • Syntax validations and translations are logically consistent and cover a comprehensive range of functionalities.

3. Readability & Structure

  • The code is well-structured with consistent naming conventions and thoughtfully placed comments.
  • However:
    • The commit message is too general. Use descriptive messages (e.g., feat: implement new language syntax, test: add test cases for syntax validation) to clarify the main changes and guide reviewers.
    • It is recommended to provide clear instructions on how reviewers should approach the files, including the recommended review order, while creating the PR.
    • Documentation is lacking for the new language’s philosophy, configuration, usage, and design structure. Creating an API reference or usage guide would benefit reviewers and future users alike.

4. Standards Compatibility

  • Proper encapsulation practices are followed with effective use of classes and namespaces.
  • Standard libraries like <unordered_map> and <string_view> are used appropriately for performance and efficiency.

5. Testing

  • Clang-tidy is included in the pipeline to avoid linting errors.
  • If available, please further provide updates on testing progress with zlib and LLVM, and/or include a link to a successful CircleCI test result to confirm no linting errors indeed.

Summary

This PR demonstrates strong engineering work and adherence to ShadowDash standards. Addressing the comments on granularity, commit structure, and documentation will make the review process smoother and improve clarity for stakeholders.


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

Successfully merging this pull request may close these issues.

2 participants