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.

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

Simplify the parser #1596

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Simplify the parser #1596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1c6e804
Simplify parse_implicit_mul() and the parser
certik Aug 6, 2019
cb0e669
Update generated file
certik Aug 6, 2019
b8c896a
Get rid of rcp_static_cast()
certik Aug 6, 2019
b04d0f3
Simplify the grammar (slight speedup)
certik Aug 6, 2019
e4d4e2f
Update generated file
certik Aug 6, 2019
d7498ab
Move things together
certik Aug 6, 2019
bdcb6c9
Fix the shift/reduce conflict
certik Aug 6, 2019
ee175c1
Regenerate
certik Aug 6, 2019
7df06e6
Use better formatting
certik Aug 6, 2019
92e3c30
Regenerate
certik Aug 6, 2019
372c0ce
Simplify func rule
certik Aug 6, 2019
e254b7c
Polish the syntax
certik Aug 6, 2019
6afdbf9
Update
certik Aug 6, 2019
64f0eb7
Simplify a few lines
certik Aug 6, 2019
f0a38da
Update generated file
certik Aug 6, 2019
4f1a90d
Better formatting
certik Aug 6, 2019
5fd6c92
Update generated file
certik Aug 6, 2019
4b70f67
Enable warnings in Bison
certik Aug 6, 2019
cec1fde
Fix all warnings
certik Aug 6, 2019
f6cc107
Regenerate
certik Aug 6, 2019
5f8eea5
Consolidate semantic declaration lines
certik Aug 6, 2019
6e0ca77
Regenerate
certik Aug 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion symengine/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -8,7 +8,7 @@ endif()

if (WITH_GENERATE_PARSER)
add_custom_command(OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/parser/parser.tab.cc
COMMAND ${BISON_EXECUTABLE} -d parser.yy
COMMAND ${BISON_EXECUTABLE} -Wall -d parser.yy
DEPENDS parser/parser.yy
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/parser
)
Expand Down
11 changes: 3 additions & 8 deletions symengine/parser/parser.cpp
View file
Open in desktop
Expand Up @@ -283,20 +283,15 @@ Parser::parse_implicit_mul(const std::string &expr)
char *endptr = 0;
std::strtod(startptr, &endptr);

RCP<const Basic> num = one, sym;

// Numerical part of the result of e.g. "100x";
size_t length = endptr - startptr;
std::string lexpr = std::string(startptr, length);
num = parse_numeric(lexpr);
RCP<const Basic> num = parse_numeric(lexpr);

// get the rest of the string
lexpr = std::string(startptr + length, expr.length() - length);
if (lexpr.length() == 0) {
sym = one;
} else {
sym = parse_identifier(lexpr);
}
SYMENGINE_ASSERT(lexpr.length() > 0);
Copy link
Member

@isuruf isuruf Aug 7, 2019 •
edited

Choose a reason for hiding this comment

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

I'd rather this throw an exception even in Release mode instead of silently ignoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give an example when this can happen? I couldn't figure out any.

Copy link
Contributor Author

@certik certik Aug 7, 2019 •
edited

Choose a reason for hiding this comment

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

And if we cannot figure out when this could happen for some user input, then the assert statement is appropriate. Asserts are for ensuring that our assumptions in the code are consistent/valid, but those do not run in release mode, because we assume our code is correct in release mode.

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, there was some ambiguity in the grammar where 2e10 was parsed as an implicit mul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I can trigger it. Too bad there wasn't a test for this. All parsing tests pass. We need to test this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quite frankly, I don't like this implicit mul parsing, due to such ambiguities. Do you think people want implicit mul to be parsed?

Copy link
Member

Choose a reason for hiding this comment

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

Do you think people want implicit mul to be parsed?

Yes, julia people use this feature heavily in SymEngine.jl

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I'm no fan of implicit mul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we are at it, we should also fix #1462. We can even have a few different parsers if needed.

RCP<const Basic> sym = parse_identifier(lexpr);
return std::make_tuple(num, sym);
}

Expand Down