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

Double always with decimal #179

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Double always with decimal #179

merged 7 commits into from
Jul 28, 2021

Conversation

xbabka01
Copy link
Contributor

Hi, this is fix of bug which ocured when you wanted to create double value without decimal (example 10.0). Yaramod in this case generated int instead of double in result

Before

yaramod.double_val(10.0).get().get_text() == '10'

now

yaramod.double_val(10.0).get().get_text() == '10.0'

@xbabka01 xbabka01 marked this pull request as draft July 16, 2021 19:43
@xbabka01 xbabka01 marked this pull request as ready for review July 16, 2021 19:47
Copy link
Member

@MatejKastak MatejKastak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was playing a bit more with the numToStr function and how to make it work without adding a new argument. What do you think about this approach? I don't think there are cases when you want to have numToStr(0.0) == "0", you should not get the expected value only if you pass in some hidden flag. Considering the backward compatibility I think this should fall into "fix" category?

template <typename T>
std::string numToStr(const T num, std::ios_base &(*format)(std::ios_base&) = std::dec, bool showbase = false, bool toUpper = false)
{
	std::ostringstream os;

	os << format;

	if (toUpper)
		os << std::uppercase;

	if (showbase)
		os << std::showbase;

	os << num;

	if (std::is_floating_point<T>::value && std::floor(num) == num)
		os << ".0";

	return os.str();
}

After some tests I wonder what we should do in cases when the precision is not sufficient. By default the max precision is 6 decimal spaces, according to docs. But if you want to create a floating point number 0.1234567 the last number is truncated in the resulting rule. Maybe its out of the scope for this PR.

Wrapping up.

Sadly, I have no explicit approval for this PR. I am still curious what do others and you think about the proposed changes.

include/yaramod/utils/utils.h Outdated Show resolved Hide resolved
include/yaramod/utils/utils.h Outdated Show resolved Hide resolved
@@ -716,7 +716,7 @@ YaraExpressionBuilder hexIntVal(std::uint64_t value)
YaraExpressionBuilder doubleVal(double value)
{
auto ts = std::make_shared<TokenStream>();
TokenIt token = ts->emplace_back(TokenType::DOUBLE, value, numToStr(value));
TokenIt token = ts->emplace_back(TokenType::DOUBLE, value, numToStr(value, std::dec, false, false, true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little cryptic for me.

  • which of this flags is relevant for this call?
  • which of this is just to satisfy defaults?

However, using this implementation I don't know how to make this better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is. I changed numToStr as you suggested to automatically detect floating type instead of using flag

@xbabka01
Copy link
Contributor Author

xbabka01 commented Jul 19, 2021

After some tests I wonder what we should do in cases when the precision is not sufficient. By default the max precision is 6 decimal spaces, according to docs. But if you want to create a floating point number 0.1234567 the last number is truncated in the resulting rule. Maybe its out of the scope for this PR.

Yeah I saw that issue #177 and tried to fix it but it broken multiple other tests see this. From that I came to the conclusion that it would be better to discuss with what precision yaramod should work. And as you mentioned it is not probably out of scope of this PR

@xbabka01
Copy link
Contributor Author

Precision if fixed in commit e580ffc (#177)

include/yaramod/utils/utils.h Outdated Show resolved Hide resolved
Copy link
Member

@metthal metthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@metthal metthal merged commit 86a84f9 into avast:master Jul 28, 2021
@xbabka01 xbabka01 deleted the fix_float branch July 28, 2021 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants