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 Design Final #26

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

Conversation

waynewangyuxuan
Copy link

No description provided.

@yw5490
Copy link

yw5490 commented Dec 10, 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

Copy link

@kyotov kyotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i saw one obvious thing... i will look more carefully again

@@ -0,0 +1,76 @@

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a full blown new file instead of just adding your test?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kyotov we have fixed this in our new PR: #29

@yw5490
Copy link

yw5490 commented Dec 12, 2024

Newest PR: #29

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.

4 participants