Add shift operations for numeric values#995
Conversation
/** For shifting operations make sure both values are integer */
private def convertToInteger(loc: Location, t: Type): Result.Result[Type] =
if (t.isInt) Right(t)
else if (t.isConvertibleTo(Type.Integer)) Right(Type.Integer)
else {
val error = SemanticError.InvalidType(loc, s"cannot convert $t to an integer type")
Left(error)
} The current implementation of int value check is too much permissive, since float is convertible to int. I will completely remove the conditional check. This causes the failure: https://github.com/abh80/fpp/blob/564f25468d2be477626cae34c49a0d5d354327f5/compiler/tools/fpp-check/test/numeric_shift/shift_float_value_error.ref.txt#L1 Edit: Looks like there is another bug t <- a.commonType(e.e1.id, e.e2.id, loc)
_ <- e.op match {
case Ast.Binop.Shift(_) => convertToInteger(loc, t)
...
}The common type inference automatically promotes to Float even if one value is int which causes misleading error information. However this dosent affect the intended behavior since it's only applicable if both values are integer |
|
@bocchino I have completed the implementation and testing. Can you give the initial review before I start writing the documentation? |
|
This generally looks good. I need to see a spec before I can approve it. Can you make a separate PR to the feature branch with just the spec changes? I'll review and approve, and then we can finalize this branch. Some notes on the spec changes:
|
This is a direct solution to the issue I found
Will add another case for this |
|
I will open another spec pull request 👍 |
Fixed this and opened a new pull request for spec revision #1000 |
…ange return type to `Result[Unit]` instead of `Result[Type]`
|
Hello @bocchino, thanks for your review I have fixed them. I have noticed when you asked me to run the trace-fprime script it ran into an error, particularly because of this commit: nasa/fprime@377fd69#diff-26fa5f1841895bd45d868f4f96adb9ed201ecb61aa1b10d6825625331e16b8c5 ; Which moved the Full logs of my trace-fprime command
I have made the following changes to diff --git a/compiler/trace-fprime b/compiler/trace-fprime
index e53625d16..726f34c20 100755
--- a/compiler/trace-fprime
+++ b/compiler/trace-fprime
@@ -6,7 +6,7 @@ cd $wd
# Make is used because it is single-string and thus doesn't trip tracing's lock-file contention
fprime_generate_flags="-f -DFPRIME_SKIP_TOOLS_VERSION_CHECK=ON -DFPRIME_ENABLE_JSON_MODEL_GENERATION=ON --ut --make"
-fprime_project_paths=". ./FppTestProject/FppTest ./Ref"
+fprime_project_paths=". ./FppTestProject/FppTest ./TestDeploymentsProject/Ref"
# Ensure the FPRIME environment variable was set
@@ -37,7 +37,7 @@ do
done
# Add in tracing for the specific CI test for fpp-to-json as this seems to be particularly unhappy.
-cd ${FPRIME}/Ref/Top
+cd ${FPRIME}/TestDeploymentsProject/Ref/Top
dependencies=`fpp-depend ../build-fprime-automatic-native-ut/locs.fpp *.fpp`
fpp-to-json ${dependencies} *.fpp |
f3fa3fd to
4c2361c
Compare
|
Thanks! It looks like trace-fprime has rotted a bit, because we haven't been using it. It was an attempt to make the native image build of fpp-json work with F Prime, but we have given up on that attempt for now. I'll open a separate issue on this. |
bocchino
left a comment
There was a problem hiding this comment.
Looks good! Thanks for addressing all the comments.
Added:
Bit shift operations to numeric values
Closes #922
Tasks
Implementation
Token.scala: addedLSHIFTandRSHIFTtokens.Lexer.scala: added token parsing for new tokens.Ast.scala:ShiftTypeto encapsulate shift typesLShift extends ShiftType .....case class Shift(op: ShiftType) extends Binopto Binop.Parser.scala:expr binopnode creation for new shift tokens.analysis/CheckSemantics/CheckExprTypes.scala:convertToIntegerfor shift operations (ignoring float values).Ast.Binop.Shift(_).Value.scala:intShiftOp = (BigInt, Int) => BigIntfor shifting.private[analysis] def intShiftOp(op: Value.Binop.intShiftOp)(v: Value)(since generic binop does not support(BigInt, Int) => Bigint))Value:isNegativeandisValidShiftAmountto help inAnalysis.scala.intShiftOpmethod inPrimitiveInt,IntegerandEnumConstant.<<and>>methods toValueto perform shift operations.Analysis.scala: addedlshiftandrshiftmethod, using the new methods added toValue.CheckSemantics/EvalConstantExprs.scala: add the new caseAst.Binop.Shift(op)using the new methods added toAnalysis.util/Error.scala: Added a newInvalidShiftAmounterror toSemanticError.Tests
compiler/tools/fpp-check/test/numeric_shift: added robust test cases including edge cases and error checks.compiler/lib/src/test/input/syntax/lexer/ok/symbols.fpp: added the new<</>>symbols.fpp-syntaxtests.fpp-to-cpptests.ValueSpectests.Spec: