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

Request to merge Specs-2017-09 with master #158

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

JingyiSu
Copy link
Contributor

Added files and changes to support specifications.

JingyiSu and others added 5 commits September 13, 2017 17:20
Added SpecCaseProtoTuple.java, SpecDeclarationProtoTuple.java,
SpecMethodProtoTuple.java, SpecStatementProtoTuple.java,
SpecVariableProtoTuple.java;
Made changes on SymbolTable.java, BoaAstIntrinsics.java,
DeclarationProtoTuple.java, MethodProtoTuple.java,
StatementProtoTuple.java, VariableProtoTuple.java, and ast.proto.
Updated copyright annotations in SymbolTable.java;
changed "required" field "key" to "optional" in ast.ptoto;
added checking condition for keys in BoaAstIntrinsics.java;
changed the identifier number for spec enums start with 1000;
removed the code part for keys in DeclarationProtoTuple.java,
MethodProtoTuple.java, StatementProtoTuple.java,
VariableProtoTuple.java;
updated Ast.java.
@psybers psybers self-requested a review September 19, 2017 03:42
src/java/boa/functions/BoaAstIntrinsics.java Show resolved Hide resolved

@SuppressWarnings("rawtypes")
public static void setup(final Context context) {
BoaAstIntrinsics.context = context;
}

Copy link
Member

Choose a reason for hiding this comment

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

remove added space


names.put("specs", counter++);
members.add(new BoaProtoList(new SpecStatementProtoTuple()));

Copy link
Member

Choose a reason for hiding this comment

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

remove blank line at end of block

@SuppressWarnings("unchecked")
@FunctionSpec(name = "getSpec", returnType = "SpecDeclaration", formalParameters = { "Declaration" })
public static SpecDeclaration getSpec(final Declaration f) {

Copy link
Member

Choose a reason for hiding this comment

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

remove the blank line at the start of this (and all getSpec methods)

@@ -1,6 +1,7 @@
/*
* Copyright 2014, Hridesh Rajan, Robert Dyer,
Copy link
Member

Choose a reason for hiding this comment

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

This file no longer needs changes. Please revert all changes so this file is not listed as changed.


names.put("specs", counter++);
members.add(new SpecStatementProtoTuple());

Copy link
Member

Choose a reason for hiding this comment

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

remove blank line at the end of the block


names.put("specs", counter++);
members.add(new SpecStatementProtoTuple());

Copy link
Member

Choose a reason for hiding this comment

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

remove blank line at the end of the block

@@ -1,6 +1,7 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This file actually shouldnt be changed, because there was no 'specs' attribute added to it. Please revert all changes to this class.


/** A single method for specification */
message SpecMethod {
repeated SpecCase case = 1;
Copy link
Member

Choose a reason for hiding this comment

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

this should be named 'cases' plural as it is repeated - also update Ast.java and SpecMethodProtoTuple.java

/** A single declaration for specification */
message SpecDeclaration {
repeated Modifier modifiers = 1;
repeated Statement specs = 2;
Copy link
Member

Choose a reason for hiding this comment

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

this should be named 'statements' - also update Ast.java

@JingyiSu
Copy link
Contributor Author

Have done all the requested changes.

@@ -67,6 +67,9 @@ message Declaration {
optional ChangeKind structural_change_kind = 10;
/** Kind of change on the label: UNCHANGED, ADDED, DELETED/REMOVED, RENAMED */
optional ChangeKind label_change_kind = 11;
/** For Specs use */
optional SpecStatement specs = 12;
Copy link
Member

Choose a reason for hiding this comment

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

should this be 'repeated'? Or is there exactly 0 or 1 specification statement per declaration?

Copy link
Member

Choose a reason for hiding this comment

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

based on the naming of 'specs' (plural) I think we intended repeated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -202,6 +237,9 @@ message Statement {
optional ChangeKind structural_change_kind = 10;
/** Kind of change on the label: UNCHANGED, ADDED, DELETED/REMOVED, RENAMED */
optional ChangeKind label_change_kind = 11;
/** For Specs use */
optional SpecStatement specs = 12;
Copy link
Member

Choose a reason for hiding this comment

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

same thing - 'repeated' or optional?

Copy link
Member

Choose a reason for hiding this comment

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

based on the naming of 'specs' (plural) I think we intended repeated here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -67,6 +67,9 @@ message Declaration {
optional ChangeKind structural_change_kind = 10;
/** Kind of change on the label: UNCHANGED, ADDED, DELETED/REMOVED, RENAMED */
optional ChangeKind label_change_kind = 11;
/** For Specs use */
optional SpecStatement specs = 12;
Copy link
Member

Choose a reason for hiding this comment

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

the field number for spec fields should start at 1000.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to change the field number for these or only for the enums for specs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -202,6 +237,9 @@ message Statement {
optional ChangeKind structural_change_kind = 10;
/** Kind of change on the label: UNCHANGED, ADDED, DELETED/REMOVED, RENAMED */
optional ChangeKind label_change_kind = 11;
/** For Specs use */
optional SpecStatement specs = 12;
Copy link
Member

Choose a reason for hiding this comment

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

the field number for spec fields should start at 1000.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@psybers psybers requested a review from nguyenhoan September 27, 2017 15:16
@psybers
Copy link
Member

psybers commented Sep 27, 2017

Would it make sense to rebase this branch off the datagen branch and merge it in right after?

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

Successfully merging this pull request may close these issues.

2 participants