Skip to content

Fix #11438 MathLib error on user defined literals #5418

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.

Sign up for GitHub

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

Jump to bottom
Closed
chrchr-github wants to merge 5 commits into from
Closed

Fix #11438 MathLib error on user defined literals #5418

chrchr-github wants to merge 5 commits into from

Conversation

@chrchr-github
Copy link
Collaborator

@chrchr-github chrchr-github commented Sep 8, 2023

No description provided.

@chrchr-github
Copy link
Collaborator Author

chrchr-github commented Sep 8, 2023

@firewave
Copy link
Collaborator

firewave commented Sep 8, 2023

  • 101010b is rejected by all three mainstream compilers, so maybe we shouldn't accept it either

Then rip it out and make it fail - or replace it with a case that works. It's not like it is the first time we didn't get some things right ages ago (looking at the MacOS filesystem). 2.13 will be quite an interesting release. 😀

On a side note - we should actually be 2.13 on main since we already have a 2.12 release branch. But releases are just the most inconsistent in all the things we do and need to got that proper at some point. I let @danmar deal with that as I am barely know but I am doing in my branches...

@chrchr-github
Copy link
Collaborator Author

chrchr-github commented Sep 8, 2023

On a side note - we should actually be 2.13 on main since we already have a 2.12 release branch. But releases are just the most inconsistent in all the things we do and need to got that proper at some point. I let @danmar deal with that as I am barely know but I am doing in my branches...

I hadn't even realized that until now. So I wonder what fixes will still make it into 2.12?

@firewave
Copy link
Collaborator

firewave commented Sep 8, 2023

I hadn't even realized that until now.

As I said. That process needs to be improved...

So I wonder what fixes will still make it into 2.12?

He's been doing some point releases for the past few releases. So anything what makes sense is open for a cherry-pick. Like recent regression or crash fixes which have a limited scope. Just CC him on such changes and it is likely to be pulled in.

firewave
firewave reviewed Sep 9, 2023
View reviewed changes
lib/token.cpp
tokType(eNumber);
else if (mStr == "=" || mStr == "<<=" || mStr == ">>=" ||
(mStr.size() == 2U && mStr[1] == '=' && std::strchr("+-*/%&^|", mStr[0])))
} else if (std::isdigit((unsigned char)mStr[0]) || (mStr.length() > 1 && mStr[0] == '-' && std::isdigit((unsigned char)mStr[1]))) {
Copy link
Collaborator

@firewave firewave Sep 9, 2023

Choose a reason for hiding this comment

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

I think this logic might also exists in simplecpp. We need to keep that in sync or provide in function to get rid of the redundant code. I might be mistaken though.

Copy link
Collaborator Author

@chrchr-github chrchr-github Sep 11, 2023

Choose a reason for hiding this comment

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

I didn't see any code that is directly related.

Copy link
Collaborator

@firewave firewave Sep 11, 2023

Choose a reason for hiding this comment

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

simplecpp::Token::flags() in simplecpp.h.

There was talk about sharing this logic with some other dev but I guess that hasn't happened yet. Oops, the PR is actually still open: #4701. Also danmar/simplecpp#285.

Copy link
Collaborator Author

@chrchr-github chrchr-github Sep 11, 2023

Choose a reason for hiding this comment

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

I see. 101010b was also discussed in there. This is getting ugly.

Copy link
Collaborator

@firewave firewave Sep 11, 2023

Choose a reason for hiding this comment

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

Ugh, that string-to-number stuff also plays into this. At least I got rid of the bogus calls and added a different conversion function but I haven't removed the code from the MathLib conversion functions which does not apply to number literals yet.

Copy link
Collaborator

@firewave firewave Sep 11, 2023

Choose a reason for hiding this comment

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

Hmm. It seems I didn't add anything which is not about number literals (my memory is a mess). If I did it must have been in #4611. But I don't think it make sense to review that.

We should simply get rid of code which makes invalid test cases pass.

Copy link
Collaborator

@firewave firewave Sep 11, 2023

Choose a reason for hiding this comment

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

If you re-use any code from that other PR please credit Gerbo via Co-authored-by: at the end of the commit message.

@danmar
Copy link
Owner

danmar commented Sep 11, 2023

He's been doing some point releases for the past few releases. So anything what makes sense is open for a cherry-pick.
Like recent regression or crash fixes which have a limited scope. Just CC him on such changes and it is likely to be pulled in.

Just I just cherry pick the fixes I think are important enough..

@danmar
Copy link
Owner

danmar commented Sep 11, 2023

I will try to be more clear when I make the 2.13 release branch..

danmar
danmar reviewed Sep 11, 2023
View reviewed changes
Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

looks good to me I don't have anything to add.

@firewave
Copy link
Collaborator

firewave commented Sep 11, 2023

I will try to be more clear when I make the 2.13 release branch..

Maybe add the PR with the release stuff as an early draft (like a week or a few days). That should have a good visibility for most devs.

I definitely won't be getting into commenting on and proposing improvements to the existing release steps this year. I have why too many open things.

But let's try to stay on the topic.

@chrchr-github chrchr-github mentioned this pull request Sep 13, 2023
Merged
chrchr-github added a commit that referenced this pull request Sep 15, 2023
@chrchr-github @gerboengels
Fix #11438 MathLib error on user defined literals (#5448)
50ba506 
Based on #4701, #5418

A helper function for the `isdigit()` test should be introduced on the
simplecpp side.

Co-authored-by: gerboengels <github@gerbo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danmar danmar danmar left review comments

@firewave firewave firewave left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chrchr-github @firewave @danmar