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

Api/naming scheme #545

Merged
merged 26 commits into from
Sep 28, 2023
Merged

Api/naming scheme #545

merged 26 commits into from
Sep 28, 2023

Conversation

SSoelvsten
Copy link
Owner

More clean-up of the public API in anticipation for the release of v2.0 . This moves the final few internal pieces of Adiar into the adiar::internal namespace, defines a naming scheme, and enforces this naming scheme across the public API.

No coding style guideline is of any value, if you do not have the reason
why as part of it [Stroustrup].
On the way, I also stumbled over some other parts of the API that could use some
clean-up, such as <adiar/internal/dot.h>.
It might be an alias, but it is not a trivial one.
I never truly liked it - removing the __ actually helped a lot...
Furthermore, explain the motivation behind the file-based variant of
'domain_set' and 'domain_get'.
This also inlines the #include<adiar/assert.h> in all files that actually depend
on it. The files are probably still more dependent on the compilation order than
I would like them to, but we can solve that as we go.
@SSoelvsten SSoelvsten added ❕ breaking version_number++ ✨ code quality Uncle Bob would be proud labels Sep 27, 2023
@SSoelvsten SSoelvsten self-assigned this Sep 27, 2023
- Enforce naming scheme for all types and variables in public API.
- Ensure only necessary files are transitively included for the public API.
- Cleaned up and added documentation to BDD and ZDD constructors.
- Replaced all `std::forward<...>(...)' with the intended 'std::move(...)'.
I also added fill to ensure it is readable in a regular text editor and not only
when rendered by a Markdown processor
@SSoelvsten
Copy link
Owner Author

SSoelvsten commented Sep 27, 2023

Still missing tasks:

@SSoelvsten SSoelvsten marked this pull request as draft September 27, 2023 19:55
@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (4f38565) 97.100% compared to head (bc0d5e2) 96.907%.

❗ Current head bc0d5e2 differs from pull request most recent head 5fcdcd6. Consider uploading reports for the commit 5fcdcd6 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##              main      #545       +/-   ##
=============================================
- Coverage   97.100%   96.907%   -0.193%     
=============================================
  Files           82        83        +1     
  Lines         5587      5626       +39     
=============================================
+ Hits          5425      5452       +27     
- Misses         162       174       +12     
Files Coverage Δ
src/adiar/adiar.cpp 93.548% <100.000%> (+0.445%) ⬆️
src/adiar/bdd.h 100.000% <100.000%> (ø)
src/adiar/bdd/apply.cpp 100.000% <100.000%> (ø)
src/adiar/bdd/bdd_policy.h 100.000% <100.000%> (ø)
src/adiar/bdd/build.cpp 100.000% <100.000%> (ø)
src/adiar/bdd/count.cpp 100.000% <100.000%> (ø)
src/adiar/bdd/evaluate.cpp 100.000% <100.000%> (ø)
src/adiar/bdd/if_then_else.cpp 96.140% <100.000%> (ø)
src/adiar/bdd/pred.cpp 100.000% <100.000%> (ø)
src/adiar/bdd/quantify.cpp 100.000% <100.000%> (ø)
... and 69 more

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

@github-actions
Copy link

Benchmark Report 🟢

origin/api/naming-scheme is a regression of 0.70% (compared to origin/main).

Minimum running time (s) for 14-Queens:

origin/main origin/api/naming-scheme
422.14 425.09
Raw Data

Running times (s) for 14-Queens:

origin/main origin/api/naming-scheme
574.73 433.82
429.24 425.09
422.14 429.83
422.24 428.72

@github-actions
Copy link

Benchmark Report 🟡

origin/api/naming-scheme is a regression of 3.22% (compared to origin/main).

Minimum running time (s) for 9-Queens:

origin/main origin/api/naming-scheme
0.31 0.32
Raw Data

Running times (s) for 9-Queens:

origin/main origin/api/naming-scheme
0.34 0.36
0.33 0.34
0.31 0.35
0.35 0.36
0.31 0.37
0.32 0.34
0.33 0.34
0.33 0.32
0.33 0.32
0.35 0.33
0.34 0.34
0.43 0.35
0.35 0.36
0.35 0.37
0.36 0.35
0.34 0.32

@github-actions
Copy link

Benchmark Report 🟢

origin/api/naming-scheme is an improvement of 2.90% (compared to origin/main).

Minimum running time (s) for 12-Queens:

origin/main origin/api/naming-scheme
15.37 14.93
Raw Data

Running times (s) for 12-Queens:

origin/main origin/api/naming-scheme
16.70 17.05
17.34 18.58
16.85 17.07
16.09 15.92
16.51 18.18
17.47 15.45
15.84 15.41
15.66 16.64
16.93 16.30
16.46 17.07
15.37 16.30
15.51 14.93
15.62 15.87
16.80 16.07
15.72 16.17
16.16 15.19

@SSoelvsten SSoelvsten marked this pull request as ready for review September 28, 2023 12:46
@SSoelvsten SSoelvsten merged commit cdf3fce into main Sep 28, 2023
48 of 50 checks passed
@SSoelvsten SSoelvsten deleted the api/naming-scheme branch September 28, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❕ breaking version_number++ ✨ code quality Uncle Bob would be proud
Projects
None yet
1 participant