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

Minor GPKG/SQLite Reorganization #670

Merged
merged 6 commits into from
Nov 13, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented Nov 6, 2023

This PR aims to solve a linkage error seen when enabling tests by simplifying the sqlite wrapper public interface and aims to reorganize the SQLite wrapper into a cleaner structure. Additionally, all GPKG and SQLite functionality is now enclosed inside the ngen namespace (i.e. ngen::sqlite and ngen::geopackage).

Previous error:

../src/geopackage/libgeopackage.a(read.cpp.o): In function `geopackage::read(std::string const&, std::string const&, std::vector<std::string, std::allocator<std::string> > const&)':
/home/shengting.cui/Xia_huc01/files_huc01/ngen/src/geopackage/read.cpp:98: undefined reference to `geopackage::sqlite_iter geopackage::sqlite::query<std::string>(std::string const&, std::string const&)'
collect2: error: ld returned 1 exit status
gmake[2]: *** [test/CMakeFiles/test_geopackage.dir/build.make:139: test/test_geopackage] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2003: test/CMakeFiles/test_geopackage.dir/all] Error 2

This error most likely resulted from my accidental implementation of the templated query function inside a source file, rather than directly in the header, i.e.:

template<typename... T>
sqlite_iter query(const std::string& statement, T const&... params);

template<typename... T>
inline sqlite_iter sqlite::query(const std::string& statement, T const&... params)
{
sqlite3_stmt* stmt;
const auto cstmt = statement.c_str();
const int code = sqlite3_prepare_v2(this->connection(), cstmt, statement.length() + 1, &stmt, NULL);
if (code != SQLITE_OK) {
throw sqlite_error("sqlite3_prepare_v2", code);
}
std::vector<std::string> binds{ { params... } };
for (size_t i = 0; i < binds.size(); i++) {
const int code = sqlite3_bind_text(stmt, i + 1, binds[i].c_str(), -1, SQLITE_TRANSIENT);
if (code != SQLITE_OK) {
throw sqlite_error("sqlite3_bind_text", code);
}
}
return sqlite_iter(std::move(stmt_t(stmt)));
}

Changes

  • Namespaces
  • sqlite -> ngen::sqlite::database
  • sqlite_iter -> ngen::sqlite::database::iterator
  • sqlite_deleter -> ngen::sqlite::database::deleter
  • database::query now has only two overloads. One is query(std::string, span<std::string>), with the second argument defaulting to an empty span. This is for code deduplication. The second overload is a templated function that takes a parameter pack of strings, and constructs the span in place. This overload is only enabled when all parameter pack arguments are convertible to std::string.
  • Renamed header files to be more consistent with the source files. I opted to keep the ngen::geopackage implementations separate to help with incremental compilation, and to aid future debugging of the optimization issue PR #522 Compiler Optimization Issue #567.
  • Separate projection header and functions into ngen::srs::epsg and new header/source proj.{hpp/cpp}.
  • Resolve PR #522 Compiler Optimization Issue #567 by defining a static table of only the projections we need, rather than searching against all defined projections in boost.geometry.

Testing

All unit tests passed locally using GCC 13.1.1, Clang 16.0.6, and IntelLLVM 2023.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux
  • macOS

@PhilMiller
Copy link
Contributor

Could you please note what the linkage error was in the PR description?

hellkite500
hellkite500 previously approved these changes Nov 7, 2023
@PhilMiller
Copy link
Contributor

Because diffing is not going to line things up nicely, were there any substantive changes in the implementation of sqlite_iter/iterator or sqlite/database? I can review the code de novo, but it would be helpful to give extra attention to bits that aren't just renamed versions of exactly the same functionality.

@program--
Copy link
Contributor Author

Because diffing is not going to line things up nicely, were there any substantive changes in the implementation of sqlite_iter/iterator or sqlite/database? I can review the code de novo, but it would be helpful to give extra attention to bits that aren't just renamed versions of exactly the same functionality.

No, the only changes made were that I renamed member variables of the iterator and database classes, but the member function logic is the same. I did rename 2 member functions as well, database::has_table is database::contains, and database::column_index became database::find.

PhilMiller
PhilMiller previously approved these changes Nov 8, 2023
@program-- program-- dismissed stale reviews from PhilMiller and hellkite500 via bad989e November 9, 2023 19:08
@program-- program-- marked this pull request as draft November 9, 2023 20:20
@program--
Copy link
Contributor Author

program-- commented Nov 9, 2023

Moving back to draft because of compiling time issues... most likely #567

@program-- program-- marked this pull request as ready for review November 9, 2023 21:02
@hellkite500 hellkite500 merged commit f5e7d1c into NOAA-OWP:master Nov 13, 2023
17 of 19 checks passed
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.

PR #522 Compiler Optimization Issue
3 participants