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

Parsing User Error for InvalidToken #21

Merged
merged 77 commits into from
May 23, 2018
Merged

Conversation

adjivas
Copy link
Contributor

@adjivas adjivas commented May 18, 2018

  • Lexer captures InvalidToken and parser returns it as a error.
  • Positions can now be implemented by @l and @r tokens at Lalrpop Report errors during Telamon-gen parsing #5 .
  • The F/lex FFI part of lexer is now more documented.
  • The token position is now checked and implemented from the Lexer grammar.

Cargo.toml Outdated
@@ -4,6 +4,7 @@ build = "build.rs"
name = "telamon"
readme = "README.md"
version = "0.2.0"
autoexamples = true
Copy link
Owner

Choose a reason for hiding this comment

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

What is that for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That a confusion about the issue 5330, travis repport a warning for a unused manifest key package.autoexamples but that not same on my configuration.
I recommend to remove this line, that probably not more needed and I have solved the example at 74f6990bdeb67bcef425ed0314a875776049f32f

@@ -180,8 +180,27 @@ typedef size_t yy_size_t;
#define EOB_ACT_END_OF_FILE 1
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this file into src/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

@@ -4,5 +4,5 @@ extern crate env_logger;

fn main() {
env_logger::init();
telamon_gen::process(&mut std::io::stdin(), &mut std::io::stdout(), true);
telamon_gen::process(&mut std::io::stdin(), &mut std::io::stdout(), true).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

Can you print nicely the error on stderr and exit with -1 rather than outputing a panic! ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That did for bin/cli_gen.rs , there is another file who need this modification? Like from cc_test

/// A F/lex's token with a span.
#[derive(Copy, Clone, Debug)]
#[repr(C)]
pub struct Span<Y> {
Copy link
Owner

Choose a reason for hiding this comment

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

For cohenrency with libsyntax, call this Spanned. Span is the same structure but without the data field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

UnexpectedToken(Position, Token, Position),
}

pub type Spanned<T, P, E> = Result<(P, T, P), E>;
Copy link
Owner

Choose a reason for hiding this comment

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

No sure what is this type. Can you add documentation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

@@ -176,6 +176,17 @@ macro_rules! generated_file {
}
}

/// Pulbic includes a generates file into the current file.
#[macro_export]
macro_rules! pub_generated_file {
Copy link
Owner

Choose a reason for hiding this comment

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

Better have a single macro generated_file with two rules: ($name:ident) and (pub $name:ident).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

let mut input = fs::File::open(path::Path::new(input_path)).unwrap();
let mut output = fs::File::create(path::Path::new(output_path)).unwrap();
let input_path_str = input_path.to_string_lossy();
info!("compiling {} to {}", input_path_str, output_path.to_string_lossy());
process(&mut input, &mut output, format);
process(&mut input, &mut output, format)
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefere if the file name was stored alongside the position in the error. This way, we can use it on a Display implementation of the Error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That mean the process function should returns a type with a Error implementation? Mean, not a tuple like this aaf0a4ddb68afe687250a521941cc5df49010feb/telamon-gen/src/lib.rs#L66?

@@ -1,35 +0,0 @@
/// Function shared among examples.
Copy link
Owner

Choose a reason for hiding this comment

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

Why have you changed this file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was moved to sgemm_low.rs because common.rs isn't a binary example

@adjivas
Copy link
Contributor Author

adjivas commented May 23, 2018

When we will have more errors management, maybe we will have to consider another output style for ProcessError interface.

@ulysseB ulysseB merged commit a40d816 into ulysseB:master May 23, 2018
@adjivas adjivas deleted the parsing branch May 24, 2018 11:58
bors bot pushed a commit that referenced this pull request Aug 20, 2018
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.

2 participants