-
Notifications
You must be signed in to change notification settings - Fork 26
Separate semantic analysis and parsing #370
Conversation
higher numeric values you can't have a precedence that's less than 0, this crashes: 1 = 2 = 3 + 4*5 + 1
This automatically runs it with --release
It was only 3 lines and only used in 1 place
No known bugs this fixes, but there shouldn't be invalid expressions with a valid type.
self.declarations.typedefs.insert(id, ()); | ||
} else if ctype == Type::Void { | ||
// TODO: catch this error for types besides void? | ||
self.err(SemanticError::VoidType, location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure what I was thinking with the comment here. The error is for void i;
and things like that.
This is _such_ a hack
Avoids segfaults (in the parser) for highly nested `sizeof`. Still segfaults in the analyzer.
_technically_ they were never removed https://xkcd.com/1475/
Where is your clippy now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
self.parse_typename(ctype, location) | ||
} | ||
// TODO: I don't think this is a very good abstraction | ||
fn parse_typename(&mut self, ctype: ast::TypeName, location: Location) -> Type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this (and the following) parsing function even be in the analysis module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, why wouldn't they be? These are helper functions for other functions in analyze
, but I don't see why they would go somewhere else.
}, | ||
}; | ||
let mut storage_class = None; | ||
for (spec, sc) in &[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this loop can be rewritten to be a little clearer, maybe even with iterators.
src/analyze/mod.rs
Outdated
None => ctype = Some(Type::Int(signed)), | ||
} | ||
} | ||
// i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give this comment some context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i;
(in a scope where i
has not previously been declared) declares a new variable called i
with type int
. This 'feature' was removed in C99 but is still common in real-world code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bit that allows this in C89 is 3.5.2:
- int , signed , signed int , or no type specifiers
This is basically a rewrite of the entire parser.
It is very much a work in progress and all the tests are failing.Additionally, I think I've lost some of the unit tests during the rewrite, I need to find them in the git history. That said, most of the work for the rewrite is done at this point and everything left is just cleanup.Changes
Most of these could be broken up into separate PRs, except for the main 'separate parsing and analysis' change.
ExprType::{Add(left, right), Sub(left, right), ...}
, haveExprType::Binary(BinaryOp, left, right)
, which makes life much easier for the constant folder and backend. This also makes the code much easier to understand sinceBinaryOp
is now a struct instead of a function.declaration_specifiers
a lot. This is now sane and won't randomly miss specifiers if they occur in the wrong order. Cannot be in a separate PR.Type::Bitfield
AssignmentToken
variants to not look dumb (AddAssign
instead ofPlusAssign
)is_decl_specifier
much more reliable. The cost of course is that's it's super hacky, but it works very reliably.Metadata
when they are declared which is reused across scopes. This means the backend no longer has to have any idea of what a scope is, making it easier to do codegen. In particular, there are no more bugs where the frontend's scope is different from the backend's scope.Action Items
These should be reverted or fixed before merging.
Location
a trait instead of a type. This failed miserably.derive_more
for displays. I only used it in one trivial place, it should either take the place ofimpl Display for Expr
or be removed altogether before this is merged.I'll make a follow-up PR for this, this one is big enough.codespan
is used only for storing theFiles
table. This is kind of a waste and I should either switch tocodespan-reporting
once and for all or write my ownFiles
real quick to remove the unnecessary dependency.Lexer
needs a design decision. Right now it implementsIterator<Item = Result<Token, LexError>>
which I like because it reflects its semantic purpose. However, the parser only acceptsIterator<Item = Result<Token, CompileError>>
.Either the lexer needs to yield CompileError or there needs to be a wrapper that turns all theI went with a third option: the parser accepts any iterator overLexError
s into the more genericCompileError
.Result<Token, E: Into<CompileError>
Issues
Closes
const
to function parameter in definition #346constexpr
field from Expr #143restrict
keyword #2#139 is fixed in the parser but now crashes because cranelift hasn't implemented boolean ops (bytecodealliance/wasmtime#1133)
Addresses #59, but only as a misfeature - it ignores all of the keywords listed there.
Makes a great deal of progress towards #266.
cc @pythondude325