cc-wrapper: fix bool handling for empty and zero values #35266
Merged
Conversation
This file contains 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
Ericson2314
reviewed
Feb 21, 2018
| @@ -25,7 +25,10 @@ mangleVarBool() { | |||
| for infix in "${role_infixes[@]}"; do | |||
| local inputVar="${var/+/${infix}}" | |||
| if [ -v "$inputVar" ]; then | |||
| let "${outputVar} |= ${!inputVar}" | |||
There was a problem hiding this comment.
I think one can directly do "${!inputVar:-0}"?
| local inputValue="${!inputVar}" | ||
| # "1" in the end makes `let` return success error code when | ||
| # expression itself evaluates to zero. | ||
| let "${outputVar} |= ${inputValue:-0}" "1" |
There was a problem hiding this comment.
I've just done || true elsewhere, which I think is a bit easier to read
There was a problem hiding this comment.
The problem with || true is that we cannot distinguish between a syntax error (inputValue="00ahaha") and legitimate zero.
There was a problem hiding this comment.
Oooo good point. Maybe add a comment about that too?
Before the code would fail silently for zero values and with some output for empties. We now currently handle both via defaulting value to zero and making `let` return success error code when there's no syntax error.
|
@Ericson2314 Updated with your suggestions, thanks! |
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.
Motivation for this change
Before the code would fail silently for zero values and with some output for
empties. We now currently handle both via defaulting value to zero and making
letreturn success error code when there's no syntax error.Test case:
nix-shell -A helloexport NIX_ENFORCE_NO_NATIVE=""gcc --version(observe error)export NIX_ENFORCE_NO_NATIVE=0gcc --version(observe silent failure)Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)