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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace sexpresso library with own implementation #802

Merged
merged 4 commits into from Nov 8, 2020

Conversation

ubruhin
Copy link
Member

@ubruhin ubruhin commented Nov 1, 2020

Replacing sexpresso::parse() with our own parser in the SExpression class. See #783 for the motivation behind this change.

Because we also used sexpresso::escape() to escape characters in strings to be serialized, I also had to replace this by our own implementation. Then I saw that sexpresso::escape() even replaced ? by \?, ' by \' and the audible bell by \a (wtf?), which doesn't make sense to me 馃槈 So I removed these replacements, but only for LibrePCB 0.2.x to keep the 0.1.x file format as-is.

Positive side effect of the own implementation: It seems to be much faster than sexpresso::parse() 馃槂 Not an accurate measurement, but on my computer the background library scan now takes ~25% less time than before (tested only in debug mode).

I just hope there are no critical bugs 馃檲 I added some unit tests and tested manually by opening/saving whole projects, at least so far it seems to work properly...

@ubruhin ubruhin added enhancement refactoring file format Issues affecting the file format labels Nov 1, 2020
@ubruhin ubruhin added this to the 0.1.6 milestone Nov 1, 2020
@ubruhin ubruhin self-assigned this Nov 1, 2020
Copy link
Member

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

The parser would be a good candidate for fuzzing, e.g. using afl++. Can be done at a later point in time though.

libs/librepcb/common/fileio/sexpression.cpp Show resolved Hide resolved
@ubruhin ubruhin merged commit 371e8da into master Nov 8, 2020
@ubruhin ubruhin deleted the 783-get-rid-of-sexpresso branch November 8, 2020 14:43
ubruhin added a commit that referenced this pull request Jul 11, 2021
Replace sexpresso library with own implementation

(cherry picked from commit 371e8da)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement file format Issues affecting the file format refactoring
Development

Successfully merging this pull request may close these issues.

Get rid of "sexpresso" dependency
2 participants