refactor codebase to leverage C++17, enhance cmakelists, and improve code readability #2091
Conversation
github-roushan
force-pushed
the
big-refactor
branch
2 times, most recently
from
c3ac900 to
9685dcb
Compare
|
@sangjanai. Adding you to review the changes and push them whenever possible before this branch diverged too much from main, leaving me to do merge :( . |
|
@vansangpfiev . can you have a look at this |
Is it ready to review or still in-progress? |
I will be making progress, but for now you can review and if good merge them. This will save me for doing merge fixes everytime it diverts from main. Thanks again |
|
Also, when submitting PR, please write about the changes so that it can easy for us to review. :) If it's WIP, then please keep it as draft. |
Noted. What i want is that the commits be merged timely so as not to diverge too much from main as it becomes painful to merge them locally. But will keep this in mind |
github-roushan
force-pushed
the
big-refactor
branch
from
9912012 to
2b0c5ce
Compare
github-roushan
changed the title
|
@vansangpfiev . This is open to be reviewed |
github-roushan
changed the title
| } | ||
|
|
||
| const std::unordered_map<GGMLType, GGMLTypeTrait> kGGMLTypeTraits = { | ||
| {GGML_TYPE_F32, {.block_size = 1, .type_size = 4}}, |
There was a problem hiding this comment.
Designated initializer requires C++20, the CIs use C++17
There was a problem hiding this comment.
Working on a fix
| @@ -204,7 +205,7 @@ if(CMAKE_CXX_STANDARD LESS 17) | |||
| find_package(Boost 1.61.0 REQUIRED) | |||
| target_include_directories(${TARGET_NAME} PRIVATE ${Boost_INCLUDE_DIRS}) | |||
| else() | |||
| message(STATUS "use c++17") | |||
| message(STATUS "use c++${CMAKE_CXX_STANDARD}") | |||
There was a problem hiding this comment.
This change is not needed.
| if (result.has_error()) { | ||
| CTL_ERR(result.error()); | ||
|
|
||
| auto hardware_json_response = curl_utils::SimpleGetJson(url.ToFullPath()); |
There was a problem hiding this comment.
Just a variable name change?
| static constexpr std::array<const char*, 3> STORAGE_INFO_HEADERS = { | ||
| "#", "Total (GiB)", "Available (GiB)"}; | ||
| static constexpr std::array<const char*, 4> POWER_INFO_HEADERS = { | ||
| "#", "Battery Life", "Charging Status", "Power Saving"}; | ||
| }; | ||
| } // namespace commands |
There was a problem hiding this comment.
File doesn't end with an empty line
Description of Changes
hardware_cmd_list.ccfile by adopting C++17 features.CMakeLists.txtto dynamically determine the C++ version used, removing hardcoding of the version in the output.setupcommand from theruncommand, improving variable naming for clarity and readability.ggml.cctoggml.hfor better modularity.Fixes Issues
Self Checklist