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

Dummy lowering and node IDs #315

Merged
merged 1 commit into from
Mar 19, 2021

Conversation

g-r-a-n-t
Copy link
Member

@g-r-a-n-t g-r-a-n-t commented Mar 18, 2021

What was wrong?

We need some form of node identification for lowering. See #306.

This is an attempt to break up the mentioned PR.

How was it fixed?

  • Added a lowering module that doesn't do anything yet.
  • Renamed Spanned to Node and added an ID field.

fe/parser/src/node.rs

Lines 48 to 54 in 953b429

#[derive(Serialize, Deserialize, Debug, PartialEq, Clone)]
pub struct Node<T> {
pub kind: T,
#[serde(skip_serializing, skip_deserializing)]
pub id: NodeId,
pub span: Span,
}

  • Refactored broken code and tests. It's worth mentioning that the hacky spanned_expression helpers were removed.

ContextHarness was broken by this change, since it relies on adding attributes to the context via the span. I've disabled this with the feature fix-context-harness and will create an issue to fix this (if merging this with the failing tests sounds ok).

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@g-r-a-n-t g-r-a-n-t changed the title Dummy lowering and node ids Dummy lowering and node IDs Mar 18, 2021
@codecov-io
Copy link

Codecov Report

Merging #315 (816913a) into master (b870477) will decrease coverage by 1.12%.
The diff coverage is 95.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   93.72%   92.60%   -1.13%     
==========================================
  Files          56       56              
  Lines        3986     3774     -212     
==========================================
- Hits         3736     3495     -241     
- Misses        250      279      +29     
Impacted Files Coverage Δ
analyzer/src/errors.rs 83.33% <ø> (ø)
analyzer/src/traversal/assignments.rs 96.66% <ø> (ø)
analyzer/src/traversal/declarations.rs 96.77% <ø> (ø)
compiler/src/yul/mappers/assignments.rs 75.00% <ø> (-22.83%) ⬇️
parser/src/lib.rs 100.00% <ø> (ø)
parser/src/tokenizer/tokenize.rs 100.00% <ø> (ø)
analyzer/src/traversal/utils.rs 77.77% <50.00%> (ø)
compiler/src/yul/utils.rs 87.87% <50.00%> (-5.88%) ⬇️
parser/src/ast_traits.rs 81.52% <72.22%> (+0.87%) ⬆️
compiler/src/lib.rs 90.00% <80.00%> (+1.11%) ⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4833c3...816913a. Read the comment docs.

@g-r-a-n-t g-r-a-n-t force-pushed the dummy-lowering-and-node-ids branch 2 times, most recently from e3c1446 to f59c8ec Compare March 19, 2021 00:25
Copy link
Collaborator

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

What a huge diff! Good you separated it from the coming changes. It looks good. I don't like the disabled tests but if you say you'll track it with an issue and make sure to get them re-enabled as soon as possible then I'm ok with it given the complexity of the overall task.

This was referenced Mar 19, 2021
@g-r-a-n-t g-r-a-n-t merged commit f5e10e1 into ethereum:master Mar 19, 2021
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.

3 participants