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

Use std::optional for options #1617

Merged
merged 28 commits into from
Sep 18, 2024
Merged

Conversation

mwestphal
Copy link
Contributor

@mwestphal mwestphal commented Sep 13, 2024

Issues to open:

library/options.json Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.84%. Comparing base (743f4f7) to head (4b7e3f9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1617      +/-   ##
==========================================
+ Coverage   96.82%   96.84%   +0.02%     
==========================================
  Files         108      108              
  Lines        7643     7668      +25     
==========================================
+ Hits         7400     7426      +26     
+ Misses        243      242       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mwestphal mwestphal changed the title optional try Use std::optional for options Sep 14, 2024
library/src/options.cxx Outdated Show resolved Hide resolved
library/src/options.cxx Outdated Show resolved Hide resolved
library/src/options.cxx Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
application/F3DOptionsTools.cxx Show resolved Hide resolved
library/src/options.cxx Outdated Show resolved Hide resolved
doc/user/OPTIONS.md Outdated Show resolved Hide resolved
library/src/options.cxx Outdated Show resolved Hide resolved
library/src/window_impl.cxx Outdated Show resolved Hide resolved
Copy link
Contributor

@Meakk Meakk left a comment

Choose a reason for hiding this comment

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

Other than the two comments, that looks very good!

doc/libf3d/OPTIONS.md Show resolved Hide resolved
library/options.json Show resolved Hide resolved
@snoyer
Copy link
Contributor

snoyer commented Sep 18, 2024

IMO the class method that return all the possible names should be static getAllNames() and the instance method that returns the names that do have a value at the time should be just getNames().

Alternatively, do we actually need the filtered names method in the API in the first place?
It looks like it is only used for the python bindings and once in the app so it would be possible to:

  • inline the hasValue(name) check in the loop in F3DStarter.cxx
  • use std::copy_if and std::count_if in the Pybind code

@mwestphal
Copy link
Contributor Author

IMO the class method that return all the possible names should be static getAllNames() and the instance method that returns the names that do have a value at the time should be just getNames().

It was you suggesting to NOT use getNames() I'm fine with either.

Alternatively, do we actually need the filtered names method in the API in the first place? It looks like it is only used for the python bindings and once in the app so it would be possible to:

* inline the `hasValue(name)` check in the loop in `F3DStarter.cxx`

* use `std::copy_if` and `std::count_if` in the Pybind code

This method is needed imo.

wdyt @Meakk

@snoyer
Copy link
Contributor

snoyer commented Sep 18, 2024

It was you suggesting to NOT use getNames() I'm fine with either.

Yes I suggested static getAllNames() to contrast with getSetNames() but @Meakk rightly pointed out that getSetNames() wasn't great ("get" and "set" in the same name is meh in the programming context because the terms are loaded because of getters and setters. It's just unfortunate that it's an irregular verb and the spelling of the past participle collides with that of the infinitive getSettedNamed would not have had that issue, oh well).
However I'm not sure "valued names" is immediately obvious either :/ but maybe that's just me, in which case carry on :)

doc/user/USAGE.md Outdated Show resolved Hide resolved
library/public/options.h.in Outdated Show resolved Hide resolved
library/public/options.h.in Outdated Show resolved Hide resolved
library/src/options.cxx Outdated Show resolved Hide resolved
@mwestphal mwestphal merged commit 80b86cc into f3d-app:master Sep 18, 2024
37 checks passed
Nokse22 pushed a commit to Nokse22/f3d that referenced this pull request Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants