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

Decompiler parenthesis cleanup #1675

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions UndertaleModLib/Decompiler/DoubleToString.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,17 @@ public static string StringOf(double number)
if (PredefinedValues.TryGetValue(number, out string res))
return res;

ReadOnlySpan<char> numberAsSpan = number.ToString("G17", CultureInfo.InvariantCulture).AsSpan(); // This can sometimes return a scientific notation
ReadOnlySpan<char> numberAsSpan = number.ToString("G16", CultureInfo.InvariantCulture).AsSpan(); // This can sometimes return a scientific notation
int indexOfE = numberAsSpan.IndexOf("E".AsSpan());
if (indexOfE < 0)
return numberAsSpan.ToString();

// This converts the scientific notation to standard form/fixed point notation
// As of time of writing this comment, C# does not offer a way to print fixed point notation while preserving precision.
// You may ask "But why not use F17?". And the answer is, that for everything but R and G, the precision is hard-capped to 15 (according to MSDN: Double.ToString()).
// Thus we use G17 to keep our precision, and then manually convert this to fixed point notation.
// You may also ask "And G17? What merits the drop to G16?".
// This is because G17 is long enough to show IEEE 754 errors, which almost always make code less readable and don't affect recompilation.
// Thus we use G16, and then manually convert this to fixed point notation.
// For anyone unaware of the general algorithm: you get the exponent 'n', then move the decimal point n times to the right if it's positive / left if it's negative.
ReadOnlySpan<char> exponentAsSpan = numberAsSpan.Slice(indexOfE + 1);
int exponent = Int32.Parse(exponentAsSpan);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,16 @@ bool checkEqual(ExpressionVar a, ExpressionVar b)
}
}
}
return String.Format("{0}{1}{2} {3}", varPrefix, varName, context.DecompilingStruct ? ":" : " =", Value.ToString(context));

// Since parenthesized is now the default output of ExpressionTwo (mainly math operations)
// It is specifically safe in variable assignment to not parenthesize this (e.g. "foo = a + b")
string stringOfValue;
if (Value is ExpressionTwo expressionTwoVal)
Miepee marked this conversation as resolved.
Show resolved Hide resolved
stringOfValue = expressionTwoVal.ToStringNoParens(context);
else
stringOfValue = Value.ToString(context);

return String.Format("{0}{1}{2} {3}", varPrefix, varName, context.DecompilingStruct ? ":" : " =", stringOfValue);
}

public override Statement CleanStatement(DecompileContext context, BlockHLStatement block)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,21 @@ cast.Argument is ExpressionConstant constant &&
}
else
{
foreach (Expression exp in Arguments)
if (Arguments.Count == 1 && Arguments[0] is ExpressionTwo expTwo)
{
context.currentFunction = this;
if (argumentString.Length > 0)
argumentString.Append(", ");
argumentString.Append(exp.ToString(context));
argumentString.Append(expTwo.ToStringNoParens(context));
}
else
{
foreach (Expression exp in Arguments)
{
context.currentFunction = this;
if (argumentString.Length > 0)
argumentString.Append(", ");
argumentString.Append(exp.ToString(context));

}
}
context.currentFunction = null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public override string ToString(DecompileContext context)
{
string arg1 = Argument1.ToString(context);
string arg2 = Argument2.ToString(context);
return String.Format("({0} {1} {2})", arg1, OperationToPrintableString(Opcode), arg2);
return String.Format("{0} {1} {2}", arg1, OperationToPrintableString(Opcode), arg2);
}

public string ToStringWithParen(DecompileContext context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,74 @@ public override Statement CleanStatement(DecompileContext context, BlockHLStatem
return this;
}

public override string ToString(DecompileContext context)
private string AddParensIfNeeded(Expression argument, DecompileContext context, bool isSecond)
{
string arg = argument.ToString(context);
bool needsParens;
if (arg[0] != '(' &&
argument is not ExpressionConstant &&
arg.Contains(' ', StringComparison.InvariantCulture))
needsParens = true;
else
needsParens = false;


if (argument is ExpressionTwo argumentAsBinaryExpression)
{
int outerPriorityLevel = Opcode switch
{
UndertaleInstruction.Opcode.Mul or UndertaleInstruction.Opcode.Div => 2,
UndertaleInstruction.Opcode.Add or UndertaleInstruction.Opcode.Sub => 1,
_ => 0,
};

// First, no parentheses on this type
arg = argumentAsBinaryExpression.ToStringNoParens(context);

int argPriorityLevel = argumentAsBinaryExpression.Opcode switch
{
UndertaleInstruction.Opcode.Mul or UndertaleInstruction.Opcode.Div => 2,
UndertaleInstruction.Opcode.Add or UndertaleInstruction.Opcode.Sub => 1,
_ => 0,
};


// Suppose we have "(arg1a argOp arg1b) opcode argument2", and are wondering whether the depicted parentheses are needed
// If the argument's opcode is more highly-prioritized than our own, such as it being multiplication
// while we use addition, then no parentheses are required.
// If the argument's opcode doesn't fall into typical math rules (that is, I don't know my full order of operations)
// Assume it has lower priority and needs parentheses to clarify.
// Parentheses are also not needed for operations of the same level, especially string concatenation.
needsParens = (outerPriorityLevel > argPriorityLevel);
if (outerPriorityLevel == 0)
needsParens = true; // Better safe than sorry
Jacky720 marked this conversation as resolved.
Show resolved Hide resolved

// Non-commutative operators may still need parentheses, e.g `a - (b - c)`
bool nonCommutative = Opcode is UndertaleInstruction.Opcode.Sub or UndertaleInstruction.Opcode.Div;
if (isSecond && nonCommutative && Opcode == argumentAsBinaryExpression.Opcode)
needsParens = true;
}

return (needsParens ? String.Format("({0})", arg) : arg);
}

public string ToStringNoParens(DecompileContext context)
{
string arg1 = AddParensIfNeeded(Argument1, context, false);
string arg2 = AddParensIfNeeded(Argument2, context, true);

if (Opcode == UndertaleInstruction.Opcode.Or || Opcode == UndertaleInstruction.Opcode.And)
{
// If both arguments are a boolean type, this is a non-short-circuited logical condition
if (Type == UndertaleInstruction.DataType.Boolean && Type2 == UndertaleInstruction.DataType.Boolean)
return String.Format("({0} {1}{1} {2})", Argument1.ToString(context), OperationToPrintableString(Opcode), Argument2.ToString(context));
return String.Format("{0} {1}{1} {2}", arg1, OperationToPrintableString(Opcode), arg2);
}
return String.Format("({0} {1} {2})", Argument1.ToString(context), OperationToPrintableString(Opcode), Argument2.ToString(context));
return String.Format("{0} {1} {2}", arg1, OperationToPrintableString(Opcode), arg2);
}

public override string ToString(DecompileContext context)
{
return String.Format("({0})", ToStringNoParens(context));
}

internal override AssetIDType DoTypePropagation(DecompileContext context, AssetIDType suggestedType)
Expand Down
Loading