Nice! Some comments:

• shifts are bit operations, they assume some representation of numbers in binary and thus it should not be possible to use them on integers
• signextend should be used as part of the "higher order bits cleanup" before the operation, i.e. appendShiftOperatorCode should not take a type argument.

shifts are bit operations, they assume some representation of numbers in binary and thus it should not be possible to use them on integers

You mean signed integers?

My understanding is the following: all shift operations should work on the internal representation of numbers, which is two's complement for int.

The different shifts:

1. SHL (shift left, <<) - Shouldn't care if the input is int/uint
2. SHR (shift right, >>>) - Shouldn't care if the input is int/uint, 0 is added on the left
3. SAR (shift arithmetic right, >>) - Should care about sign, 1 is added on the left if it is signed (i.e. highest bit was 1, depends on the width of intXX)

The above is taken from Javascript and is what the Solidity parser had. I am not fully convinced we need SAR - if omit it, I would suggest to have >> for SHR.

I would hold off on this until after the fixed point and rationals are implemented....there are some major changes that kind of dork what you're trying to do in your set up :)

@axic no I mean integers in themselves. There is no such thing as "shifting an integer", it is just a number, you should not have to assume that it is in two's complement. At the point where you explicitly convert the integer into a bytesNN, it assumes two's complement representation, the notions of "left" and "right" get a meaning and it makes sense to "shift" in those directions.

Having said that: I realize how used people are to applying shift operators to integers...

So if we add shift operators, I would prefer to drop >>> and use x >> y to be identical to x / 2**y.

One advantage of that is that we can say that "shifting an integer" means multiplying / dividing by appropriate powers of two and thus get rid of the number representation issue.

Concerning the break with javascript: Solidity is typed, so the distinction between >> and >>> is not really needed.

consider getting rid of the >>> token?

@chriseth It is more clear now. Coming from a C background, I only have a notion of "integers" which behave like "bytesNN".

Do I understand it right that you want a left and right shift operator, denoted by << (SHL) and >> (SHR) respectively and that it behaves according to the type information. For signed integers the right shift would behave like an arithmetic right shift (= x / 2**y).

Is this correct?

I think @VoR0220 added support for shift operations on constants as a PR to my branch. I'll merge that soon.

Following up with the previous question. Do we agree with the following?

• Have logical shifts only: << (SHL) and >> (SHR)
• Remove the SHR define
• Remove the >>> token

If yes, I'll push an update later today.

As per our discussion, for the moment the following assumptions are made:

• >>> token is kept, but an exception is thrown in Types/ExpressionCompiler
• SHL, SHR, SAR defines are unchanged, but internally we'll only use SHL and SAR which denote << and >> respectively.
• shifting with negative right value is prohibited (throws an error)

@chriseth

Should I change this to include Token::isShiftOp(c_op)?

@@ -363,7  363,7 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
                bool cleanupNeeded = false;
                if (Token::isCompareOp(c_op))
                        cleanupNeeded = true;
-               if (commonType.category() == Type::Category::Integer && (c_op == Token::Div || c_op == Token::Mod))
                if (commonType.category() == Type::Category::Integer && (c_op == Token::Div || c_op == Token::Mod || Token::isShiftOp(c_op)))
                        cleanupNeeded = true;

                // for commutative operators, push the literal as late as possible to allow improved optimization

Also where do I add the check for negative right value? The same visitor? After cleanup? Something like this?

@@ -386,6  386,11 @@ bool ExpressionCompiler::visit(BinaryOperation const& _binaryOperation)
                        leftExpression.accept(*this);
                        utils().convertType(*leftExpression.annotation().type, commonType, cleanupNeeded);
                }
                if (Token::isShiftOp(c_op))
                {
                        IntegerType const& type = dynamic_cast<IntegerType const&>(*rightExpression.annotation().type);
                        solAssert(!type.isSigned(), "Shift with negative right value is not supported.");
                }
                if (Token::isCompareOp(c_op))
                        appendCompareOperatorCode(c_op, commonType);
                else

I think it would be good to perform cleanup before every shift operation. The cleanup for the left operand is not necessary for left shifts, but I think it is fine to do it still.

The check that the right operand is not negative has to be done in asm. Even if the right operand has a signed type, it can still be positive.

Finally, I think we should ensure that the second operand has to be an integer (i.e. not a bytesN) - please add a test for that, not sure if we already do.

I think it is good to promote the appendShiftOperatorCode function out of appendOrdinaryBinaryOperatorCode and call it at the same point where that is called because it requires some type information, at least for the second operand.

@axic what's the status of this?

@VoR0220 I've been away, will look into my open Solidity issues this week.

@chriseth

Any idea how this could be broken?

/home/travis/build/ethereum/solidity/test/../test/libsolidity/SolidityExecutionFramework.h(71): fatal error in "bytes_index_access": Unknown exception thrown by m_compiler.compile(m_optimize, m_optimizeRuns)

Also, how to test with signed ints? There doesn't seem to be an i256(). Only fromHex() is the way?

Removed the catch-all and I get this:

libc  abi.dylib: terminating with uncaught exception of type std::bad_cast: std::bad_cast

@chriseth:

The check that the right operand is not negative has to be done in asm.

Do we check that in every single case or only if it a signed type?

I've added a piece of code to appendShiftOperatorCode, which invokes the errorTag.

@axic signed integers: just use u256(-1).
The test for negativeness has of course only be done for signed types.

Add tests for:

• smaller types
• checks that high order bit cleanup is performed correctly
• right shift of negative left hand sides
• shifts by negative amounts (both for signed and unsigned right hand sides)
• more complex usage of shifts in some actually useful snippet
• shift assignment (a <<= 2)

I'll squash some of the commits and rebase against develop later.

I added a test case about cleaning up higher bits.

Closing in Favour of #1487 (with a branch in this repository).