Repository navigation
parser: fix fixed array with eval const size (fix #19265) - #19269
Merged
Merged
Conversation
spytheman
reviewed
Sep 4, 2023
Comment on lines
+872
to
+876
| if p.pref.is_fmt { | ||
| // for vfmt purposes, pretend the constant does exist | ||
| // it may have been defined in another .v file: | ||
| return 1, false | ||
| } |
Contributor
There was a problem hiding this comment.
that should be the first check in that function, since it is very quick and it will save you 1 level of indentation for the rest of the body
Member
Author
There was a problem hiding this comment.
The original processing logic does this only if this constant value is not found.
spytheman
reviewed
Sep 4, 2023
Comment on lines
+34
to
+68
| ast.InfixExpr { | ||
| mut show_non_const_error := false | ||
| mut left_val := 1 | ||
| mut right_val := 1 | ||
| if size_expr.left is ast.Ident { | ||
| left_val, show_non_const_error = p.check_and_eval_const_val(size_expr.left) | ||
| if show_non_const_error { | ||
| p.error_with_pos('non-constant array bound `${size_expr.left.name}`', | ||
| size_expr.left.pos) | ||
| } | ||
| } | ||
| if size_expr.right is ast.Ident && !show_non_const_error { | ||
| right_val, show_non_const_error = p.check_and_eval_const_val(size_expr.right) | ||
| if show_non_const_error { | ||
| p.error_with_pos('non-constant array bound `${size_expr.right.name}`', | ||
| size_expr.right.pos) | ||
| } | ||
| } | ||
| match size_expr.op { | ||
| .plus { | ||
| fixed_size = left_val + right_val | ||
| } | ||
| .minus { | ||
| fixed_size = left_val - right_val | ||
| } | ||
| .mul { | ||
| fixed_size = left_val * right_val | ||
| } | ||
| .div { | ||
| fixed_size = left_val / right_val | ||
| } | ||
| else { | ||
| p.error_with_pos('fixed array size cannot use non-constant value', | ||
| size_expr.pos) | ||
| } |
Contributor
There was a problem hiding this comment.
Reusing the evaluation code from the transformer may be shorter:
mut t := transformer.new_transformer(p.pref)
folded_expr := t.infix_expr(mut size_expr)It already does the recursion, and you are using it for the left/right branches of the expression anyway.
Pinpointing the exact branch, where the constant was used is not worth the complications in the parser; using just size_expr.pos for the error is enough.
Member
Author
There was a problem hiding this comment.
transformer can not work for const value in infix expr.
Member
Author
|
I'll use transformer to optimize it in another PR. mut t := transformer.new_transformer(p.pref)
folded_expr := t.expr(mut size_expr) |
spytheman
added a commit
to Spydr06/v
that referenced
this pull request
Sep 8, 2023
* master: (128 commits) ci: update containers_ci.yml to not try to install libexecinfo-static, which is not present on the latest Alpine image checker: disallow module name duplicates in local names (vlang#18118) cgen: cleanup go_before_stmt(0) (vlang#19308) parser: disallow using `sql` as name (vlang#19298) fmt: cleanup fmt comments (vlang#19306) tests: supplement test cases in fixed_array_const_size_test.v (vlang#19303) vdoc: prevent main-content outline with certain devices / browsers (vlang#19304) cgen: fix printing struct with thread field (vlang#19302) vlib: replace macros that resolve to __builtin_bswapnn calls for tcc (vlang#19305) time: add `MMM` in the doc comment for parse_format() (vlang#19299) os: include sys/sysctl.h on FreeBSD to avoid implicit definition of sysctl function (vlang#19293) parser, transformer: fix transformer.infix_expr() and cleanup parse_types.v (related vlang#19269) (vlang#19276) fmt: simplify the processing logic for removing inline comments (vlang#19297) db.pg: allow postgres connection using service definitions (vlang#19288) builtin: use `libgc-threaded` on FreeBSD, to get the threaded version of libgc (vlang#19294) ci: test the pure V math versions without .c.v overrides on the CI too (vlang#19292) all: support `./v -exclude @vlib/math/*.c.v vlib/math/math_test.v`, for using the pure V math module implementation, without the .c.v overrides there. (vlang#19290) .cirrus.yml: change test_zip_modules -> test_zip_modules_script, so that the CI can recognise the new task name and execute it thirdparty/zip: properly include utime.h and set defines for FreeBSD (vlang#19285) math: fix pure v math.pow (vlang#19287) ...
Wertzui123
pushed a commit
to Wertzui123/v
that referenced
this pull request
Oct 8, 2023
Wertzui123
pushed a commit
to Wertzui123/v
that referenced
this pull request
Oct 8, 2023
mwmuni
added a commit
to mwmuni/v
that referenced
this pull request
Nov 5, 2023
…nd cleanup parse_types.v (related vlang#19269) (vlang#19276)" This partially reverts commit 055e113.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fix fixed array with eval const size (fix #19265).