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

refactor option handling #1351

Merged
merged 26 commits into from
Oct 18, 2024
Merged

refactor option handling #1351

merged 26 commits into from
Oct 18, 2024

Conversation

tsteven4
Copy link
Collaborator

This simplifies option usage and related memory management. It also resolves #982. I originally anticipated additional derived classes of Option for other argument types, but this is quite a large changeset already.

Copy link
Collaborator

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

This is awesome!

I poked at this a few years ago and kept collapsing into my own mass. This cleans up a lot of really funky edge casese for us and makes the code way cleaner.

I hope that CLion (or Neovim or something...) helped you with that bulk editing. Yuk!

Thanx for taking this on.

@@ -460,7 +461,7 @@ class LowranceusrFormat : public Format
int trail_point_count{};
bool merge_new_track{false};
short num_section_points{};
char* merge{};
OptionBool merge;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It wouldn't make me sad to use this whitespace convention (the one we use everywhere else) throughout these couple of blocks.

*/
explicit(false) operator const char* () const
{
return value_.isNull()? nullptr : valueb_.constData();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel bad that the most impactful thing I have to say is about whitespace...

option.h Show resolved Hide resolved
@tsteven4
Copy link
Collaborator Author

I poked at this a few years ago and kept collapsing into my own mass.

This is my third or fourth round at this over the years, and I started this round twice. There was a lot of experimentation, some thought, and a little learning to get here. A key detail is how implicit conversions work, especially the "Order of the conversions". A goal was to allow the implicit conversions we historically have relied on while throwing compile errors for undesired usages. Most of the changes in the .cc files are in response to desired compile errors. Some, like comparing OptionBool to nullptr, which I put in in response to clang-tidy over the years, just make the code more readable. Others, like the usages of OptionCString::get, actually caught and eliminated round trips from QString -> c string -> QString, or corrected bugs were we printed an error message in utf-8 instead of local 8 bit. Adding too many user defined conversion functions to a class can lead to ambiguous conversion compile errors. In the current design this was eliminated by only defining one user defined conversion function per concrete class. It can also be dealt with by using explicit conversions.

Unfortunately the editing was all done by hand. CLion on one machine helped trace overladed relational operators of a specific class that didn't make it to the final design, but strangely didn't work at all on a second machine with the same version of CLion or on another with Resharper.

This opens the door for formats & filters to eliminate c strings one format or filter at a time. But before we embark on that journey I want to consider another concrete class OptionString that has different user defined conversion function(s) than OptionCString, and drops OptionCString's c string support. We are also down to THREE remaining uses of xstrdup, and none are anywhere near as difficult to remove as the one in Vecs this PR removed. I intend to leave these issues for subsequent PRs.

@tsteven4 tsteven4 merged commit 8466bfb into GPSBabel:master Oct 18, 2024
18 checks passed
@tsteven4 tsteven4 deleted the classoption branch October 18, 2024 21: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.

garmin, ozi, xcsv mkshort options whitespace_ok, must_unique broken since 2006
2 participants