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

Enable sentences with features #91

Closed
elpiel opened this issue May 9, 2023 · 7 comments
Closed

Enable sentences with features #91

elpiel opened this issue May 9, 2023 · 7 comments

Comments

@elpiel
Copy link
Member

elpiel commented May 9, 2023

We should have all sentences enabled with a feature, e.g. "all-sentences", however, it should be possible to parse only a subset of sentences by enabling only their own feature, e.g. gll or gsv, etc.

This will ensure that the size of the crate is optimised for highly constraint targets which require some subset of the sentences.

This is related to #54

@PegasisForever
Copy link

I guess this will be a breaking change? If we make all-sentences the default feature, applications use default-features = false for no-std support will break

@elpiel
Copy link
Member Author

elpiel commented May 10, 2023

Valid point. Yes it will be, but we'll bump version to 0.5.
We can also include Changelog but I'm not afraid to break the build from minor version updates.

default = ["std", "all-sentences"]
all-sentences = ["...", "vendor-specific"]
vendor-specific = [".."]
...
..
.

@Dushistov
Copy link
Collaborator

What about usage of macros instead of features?

Something like

generate_parser![SentenceType::AAM, SentenceType::ABK];

and that generate nmea parser that parse only AAM and ABK messages,
so compiler+linker in release mode can remove unused part of nmea crate?

Plus it would be nice implement the similar fnctionality for Nmea::create_for_navigation.

This variant of dealing with redundant code allows the compiler to solve the unnecessary code problem,
instead of manually marking of each sentence with feature.

@elpiel
Copy link
Member Author

elpiel commented May 14, 2023

@Dushistov macros might make it more difficult to use the crate and contribute to the project IMO.
Also, not all tools/ides/editors play nicely with macros.
On no_std crates, I've seen macros used to implement different peripherals but not exposing the internal macros for users to define their own.

It's tedious process to add these features now but explicitly listing them sounds like a very good approach.
Due to the way parsers work, I suppose we can even add "Feature not enabled" error when a sentences is passed which can be parsed but the feature hasn't been enabled.

@elpiel
Copy link
Member Author

elpiel commented May 14, 2023

There's one more additional benefit that I though of.
We can leave the parsers themselves and just exclude the calling of the parser in the parse_str function.

Compiler will still optimize and remove the unused parser/code, however, if you want to parse a specific sentences in your project which you have excluded from parse_str, then it's your choice and you can do it without enabling the feature.

@ekuinox
Copy link

ekuinox commented May 14, 2023

In my case, I use my own ParseResult-like enumeration type combined with some public parsers instead of using ParseResult. So I think it is a good change to be able to exclude unused sentences from the compilation by means of a feature flag.

@elpiel
Copy link
Member Author

elpiel commented Jul 20, 2023

These sentences features have been implemented in #94 .
0.5 release introduce this breaking change, however, we do not have guarantees for keeping the API for minor version bump and we do have other breaking changes as well so this is ok.

Closing this issue for now as it's Done ✔️ .
If anyone has suggestions on how to improve those don't hesitate to open issues or PR!

@elpiel elpiel closed this as completed Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants