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

♻️ vendor logicblocks #424

Merged
merged 8 commits into from
Feb 12, 2024
Merged

♻️ vendor logicblocks #424

merged 8 commits into from
Feb 12, 2024

Conversation

burgholzer
Copy link
Member

@burgholzer burgholzer commented Feb 8, 2024

Description

This PR was born out of frustration with the implementation of the LogicBlocks library as well as the fact that maintaining it as a submodule has lots of disadvantages (no shared CMake configuration) for little to no benefits.

So this PR does two things:

  • it eliminates the LogicBlocks submodule completely and instead vendors the code directly as part of QMAP.
  • it significantly refactors and strips down the corresponding implementation. Mostly, it replaces pImpl patterns with standard C++ code. The code is still a mess, but much less so then before.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

Signed-off-by: burgholzer <burgholzer@me.com>
@burgholzer burgholzer added dependencies Pull requests that update a dependency file submodules Pull requests that update Submodules code usability Anything related to usability c++ Anything related to C++ code enhancement Anything related to improvements of the existing library minor Changes leading to a minor version increase labels Feb 8, 2024
@burgholzer burgholzer self-assigned this Feb 8, 2024
};

struct WeightedVar {
WeightedVar(LogicTerm v, const int w) : var(std::move(v)), weight(w) {}

Check notice

Code scanning / CodeQL

Large object passed by value

This parameter of type [LogicTerm](1) is 128 bytes - consider passing a const pointer/reference instead.

struct NestedVar {
explicit NestedVar(LogicTerm v) : var(std::move(v)), list(){};
NestedVar(LogicTerm v, std::vector<NestedVar> l)

Check notice

Code scanning / CodeQL

Large object passed by value

This parameter of type [LogicTerm](1) is 128 bytes - consider passing a const pointer/reference instead.
using namespace logicbase;

struct NestedVar {
explicit NestedVar(LogicTerm v) : var(std::move(v)), list(){};

Check notice

Code scanning / CodeQL

Large object passed by value

This parameter of type [LogicTerm](1) is 128 bytes - consider passing a const pointer/reference instead.
enum class Type { Uninitialized, AuxVar, ProgramVar };
struct SavedLit {
SavedLit() : var(LogicTerm::noneTerm()) {}
SavedLit(Type t, LogicTerm v) : type(t), var(std::move(v)) {}

Check notice

Code scanning / CodeQL

Large object passed by value

This parameter of type [LogicTerm](1) is 128 bytes - consider passing a const pointer/reference instead.
src/logicblocks/LogicTerm.cpp Fixed Show fixed Hide fixed
src/logicblocks/LogicTerm.cpp Fixed Show fixed Hide fixed
src/logicblocks/LogicTerm.cpp Fixed Show fixed Hide fixed
return z3base->variables.at(id)[static_cast<size_t>(type)].second;
}

z3::expr Z3Base::convert(const LogicTerm& a, CType toType) {

Check warning

Code scanning / CodeQL

Poorly documented large function

Poorly documented function: fewer than 2% comments for a function of 149 lines.
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

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

Comparison is base (b4c29ea) 92.7% compared to head (5c58a62) 89.8%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #424     +/-   ##
=======================================
- Coverage   92.7%   89.8%   -3.0%     
=======================================
  Files         48      61     +13     
  Lines       5340    6340   +1000     
  Branches     910    1008     +98     
=======================================
+ Hits        4952    5694    +742     
- Misses       388     646    +258     
Flag Coverage Δ
cpp 89.4% <76.4%> (-3.2%) ⬇️
python 94.4% <ø> (-0.1%) ⬇️
Files Coverage Δ
include/cliffordsynthesis/Results.hpp 100.0% <ø> (ø)
include/cliffordsynthesis/Tableau.hpp 86.6% <ø> (-10.8%) ⬇️
include/cliffordsynthesis/encoding/GateEncoder.hpp 88.8% <ø> (-11.2%) ⬇️
...de/cliffordsynthesis/encoding/ObjectiveEncoder.hpp 100.0% <100.0%> (ø)
include/cliffordsynthesis/encoding/SATEncoder.hpp 100.0% <ø> (ø)
...lude/cliffordsynthesis/encoding/TableauEncoder.hpp 100.0% <ø> (ø)
include/configuration/Configuration.hpp 100.0% <ø> (ø)
include/logicblocks/Encodings.hpp 100.0% <100.0%> (ø)
include/logicblocks/LogicTerm.hpp 100.0% <100.0%> (ø)
include/logicblocks/Model.hpp 100.0% <100.0%> (ø)
... and 22 more

... and 7 files with indirect coverage changes

Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
Signed-off-by: burgholzer <burgholzer@me.com>
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
test/test_logicblocks.cpp Fixed Show fixed Hide fixed
Signed-off-by: burgholzer <burgholzer@me.com>
@burgholzer
Copy link
Member Author

Ignoring the decrease in coverage here in favor of the simplicity of the resulting solution.

@burgholzer burgholzer merged commit 6c9d6ee into main Feb 12, 2024
33 of 35 checks passed
@burgholzer burgholzer deleted the vendor-logicblocks branch February 12, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Anything related to C++ code dependencies Pull requests that update a dependency file enhancement Anything related to improvements of the existing library minor Changes leading to a minor version increase submodules Pull requests that update Submodules code usability Anything related to usability
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

1 participant