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

Fix grammar verification #37607

Merged
merged 1 commit into from
Nov 17, 2016
Merged

Fix grammar verification #37607

merged 1 commit into from
Nov 17, 2016

Conversation

dns2utf8
Copy link
Contributor

@dns2utf8 dns2utf8 commented Nov 5, 2016

  • Use make check-lexer to verify the grammar.
  • Extend grammar/README
  • Add make clean-grammar rule
  • Add target check-build-lexer-verifier to make tidy, so it will build the verifier with every build and catch future errors

This is the continuation of #34994

r? @steveklabnik @jonathandturner @alexcrichton

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Nice! Hopefully we'll be able to easily get a "bot" up and running for this soon once we switch to Travis

@@ -37,7 +37,7 @@ $(BG):

$(BG)RustLexer.class: $(BG) $(SG)RustLexer.g4
$(Q)$(CFG_ANTLR4) -o $(BG) $(SG)RustLexer.g4
$(Q)$(CFG_JAVAC) -d $(BG) $(BG)RustLexer.java
$(Q)$(CFG_JAVAC) -d $(BG) -classpath /usr/share/java/antlr-complete.jar $(BG)RustLexer.java
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to abstract this to avoid hardcoding it?

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 you mean something like this?

echo $(find /usr/ -name antlr-complete.jar 2>/dev/null)

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking more of a ./configure style thing (or detection elsewhere)

@steveklabnik
Copy link
Member

Cool!

@alexcrichton
Copy link
Member

@dns2utf8 thoughts about configuring where antlr itself is located given my previous comment?

@@ -20,11 +20,11 @@ skipped=0
check() {
grep --silent "// ignore-lexer-test" "$1";

# if it's *not* found...
# if it is *not* found...
Copy link
Member

Choose a reason for hiding this comment

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

Not sure to see the usefulness of this change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I read it and I thought I change it. Is it wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Well it's not wrong, but "it's is just as good as "it is" so why changing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I learned it that way in school, an old habit.

if [ $? -eq 1 ]; then
cd $2 # This `cd` is so java will pick up RustLexer.class. I couldn't
cd $2 # This `cd` is so java will pick up RustLexer.class. I could not
Copy link
Member

Choose a reason for hiding this comment

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

Same.

# figure out how to wrangle the CLASSPATH, just adding build/grammar
# didn't seem to have any effect.
# did not seem to have any effect.
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@dns2utf8
Copy link
Contributor Author

@alexcrichton I have never worked with the configure-step. I am looking into it now.

@@ -852,6 +852,7 @@ probe_need CFG_CMAKE cmake
# probe for it only in this case.
if [ -n "$CFG_ANTLR4" ]
then
putvar "CFG_ANTLR4_JAR $(echo -n $(find /usr/ -name antlr-complete.jar 2>/dev/null | head -n 1))"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a strange bug: With this line i save the CFG_ANTLR4_JAR to the generated config but it also generates an additional line after this entry.
That entry is empty and stops the use of the config.mk.

Do you have an idea?

Copy link
Member

Choose a reason for hiding this comment

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

Is that perhaps because the quote after JAR isn't terminated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was unable to fix the bug inline without creating another one. So I rewrote it with a unfortunately global variable.

@dns2utf8
Copy link
Contributor Author

I extended the find to search the users homedirectory for antlr-complete.jar if it is installed on the system.
I do not have access to a mac, but I hope this will work there too.

 * Use `make check-lexer` to verify the grammar.
 * Extend grammar/README
 * Add make clean-grammar rule
 * Add target `check-build-lexer-verifier` to `make tidy`, so it will build the verifier with every build and catch future errors
 * Search for antlr4 with configure and find
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 17, 2016

📌 Commit 0e1828a has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 17, 2016

⌛ Testing commit 0e1828a with merge 6cd5be8...

bors added a commit that referenced this pull request Nov 17, 2016
Fix grammar verification

 * Use make check-lexer to verify the grammar.
 * Extend grammar/README
 * Add make clean-grammar rule
 * Add target check-build-lexer-verifier to make tidy, so it will build the verifier with every build and catch future errors

This is the continuation of #34994

r? @steveklabnik @jonathandturner @alexcrichton
@bors bors merged commit 0e1828a into rust-lang:master Nov 17, 2016
@dns2utf8 dns2utf8 deleted the doc_grammar branch November 17, 2016 09:55
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.

5 participants