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

assembler: Error if extra args are present in pragmas #5400

Merged
merged 3 commits into from
May 19, 2023

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented May 18, 2023

Summary

One might think these programs are equivalent:

#pragma version 5; int 1
#pragma version 5
int 1

However, they are not. Pragma lines are "greedy" in that they consume all tokens in a line, but only look at the first 3.

I've ran into this a few times, so I thought it would be a good idea to at least error if you try to assemble with extra stuff after a pragma.

A better solution would be to treat semicolons as newlines and make those example programs actually equal, but I've not gone that far in this PR. (And it deserves some extra thought, since it's currently illegal to start a directive unless it's the first thing in a line. Plus #define is greedy because it actually consumes the whole line.)

Test Plan

Unit test added, existing tests fixed.

@jasonpaulos jasonpaulos self-assigned this May 18, 2023
@jasonpaulos jasonpaulos marked this pull request as ready for review May 19, 2023 00:39
@codecov
Copy link

codecov bot commented May 19, 2023

Codecov Report

Merging #5400 (6bd9c70) into master (b376efd) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5400      +/-   ##
==========================================
+ Coverage   55.38%   55.43%   +0.05%     
==========================================
  Files         452      452              
  Lines       63685    63689       +4     
==========================================
+ Hits        35269    35308      +39     
+ Misses      25990    25967      -23     
+ Partials     2426     2414      -12     
Impacted Files Coverage Δ
data/transactions/logic/assembler.go 90.59% <100.00%> (+0.24%) ⬆️
netdeploy/remote/deployedNetwork.go 18.88% <100.00%> (ø)

... and 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

This looks good. You've just invited yourself to be a reviewer on my big PR that changes a lot of error handling though... I'll have it up in an hour or so.

Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

This looks good. You've just invited yourself to be a reviewer on my big PR that changes a lot of error handling though... I'll have it up in an hour or so.

@jannotti jannotti merged commit 5e1f8a2 into algorand:master May 19, 2023
@jasonpaulos jasonpaulos deleted the assembler-pragma-extra-args branch May 19, 2023 16:15
tzaffi pushed a commit to tzaffi/go-algorand that referenced this pull request May 22, 2023
wip

netdeploy: Copy ledger directory for kv tracker database (algorand#5392)

assembler: Error if extra args are present in pragmas (algorand#5400)

ledger: Exclude stake at R-320 that is expired by R (algorand#5403)

Co-authored-by: cce <[email protected]>

algod: Don't return a top level array from algod (algorand#5404)

revert

go get github.com/algorand/[email protected]

go get github.com/getkin/[email protected]

go mod tidy -compat=1.17

go get github.com/algorand/[email protected]

go mod tidy -compat=1.17

go get github.com/algorand/[email protected]

revert to master

github.com/getkin/kin-openapi v0.117.0

github.com/getkin/kin-openapi v0.117.0

github.com/getkin/kin-openapi v0.117.0

go get golang.org/x/mod/modfile

tidy #0

go get github.com/algorand/[email protected]

go mod tidy

tidy up after every go get

simplify

revert

tidier

tg

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

Successfully merging this pull request may close these issues.

3 participants