Copilot commented on code in PR #847:
URL: https://github.com/apache/incubator-graphar/pull/847#discussion_r2786208800
##########
cpp/thirdparty/simple-uri-parser/uri_parser.h:
##########
@@ -27,215 +27,222 @@
#ifndef SIMPLE_URI_PARSER_LIBRARY_H
#define SIMPLE_URI_PARSER_LIBRARY_H
+#include <algorithm>
#include <string>
#include <unordered_map>
-#include <algorithm>
#ifndef simple_uri_CPLUSPLUS
-# if defined(_MSVC_LANG ) && !defined(__clang__)
-# define simple_uri_CPLUSPLUS (_MSC_VER == 1900 ? 201103L : _MSVC_LANG )
-# else
-# define simple_uri_CPLUSPLUS __cplusplus
-# endif
+#if defined(_MSVC_LANG) && !defined(__clang__)
+#define simple_uri_CPLUSPLUS (_MSC_VER == 1900 ? 201103L : _MSVC_LANG)
+#else
+#define simple_uri_CPLUSPLUS __cplusplus
+#endif
#endif
-#define simple_uri_CPP17_OR_GREATER ( simple_uri_CPLUSPLUS >= 201703L )
+#define simple_uri_CPP17_OR_GREATER (simple_uri_CPLUSPLUS >= 201703L)
namespace uri {
#if simple_uri_CPP17_OR_GREATER
- using string_view_type = std::string_view;
- using string_arg_type = std::string_view;
- constexpr auto npos = std::string_view::npos;
+using string_view_type = std::string_view;
+using string_arg_type = std::string_view;
+constexpr auto npos = std::string_view::npos;
#else
- using string_view_type = std::string;
- using string_arg_type = const std::string&;
- constexpr auto npos = std::string::npos;
+using string_view_type = std::string;
+using string_arg_type = const std::string&;
+constexpr auto npos = std::string::npos;
#endif
using query_type = std::unordered_map<std::string, std::string>;
enum class Error {
- None,
- InvalidScheme,
- InvalidPort,
+ None,
+ InvalidScheme,
+ InvalidPort,
};
struct Authority {
- std::string authority;
- std::string userinfo;
- std::string host;
- long port = 0;
+ std::string authority;
+ std::string userinfo;
+ std::string host;
+ long port = 0;
};
struct Uri {
- Error error;
- std::string scheme;
- Authority authority = {};
- std::string path;
- query_type query = {};
- std::string query_string;
- std::string fragment;
-
- explicit Uri(Error error) : error(error) {}
- Uri(std::string scheme, Authority authority, std::string path, query_type
query, std::string query_string, std::string fragment)
- : error(Error::None)
- , scheme(std::move(scheme))
- , authority(std::move(authority))
- , path(std::move(path))
- , query(std::move(query))
- , query_string(std::move(query_string))
- , fragment(std::move(fragment))
- {}
+ Error error;
+ std::string scheme;
+ Authority authority = {};
+ std::string path;
+ query_type query = {};
+ std::string query_string;
+ std::string fragment;
+
+ explicit Uri(Error error) : error(error) {}
+ Uri(std::string scheme, Authority authority, std::string path,
+ query_type query, std::string query_string, std::string fragment)
+ : error(Error::None),
+ scheme(std::move(scheme)),
+ authority(std::move(authority)),
+ path(std::move(path)),
+ query(std::move(query)),
+ query_string(std::move(query_string)),
+ fragment(std::move(fragment)) {}
};
-}
+} // namespace uri
namespace {
bool valid_scheme(uri::string_arg_type scheme) {
- if (scheme.empty()) {
- return false;
- }
- auto pos = std::find_if_not(scheme.begin(), scheme.end(), [&](char c){
- return std::isalnum(c) || c == '+' || c == '.' || c == '-';
- });
- return pos == scheme.end();
+ if (scheme.empty()) {
+ return false;
+ }
+ auto pos = std::find_if_not(scheme.begin(), scheme.end(), [&](char c) {
+ return std::isalnum(c) || c == '+' || c == '.' || c == '-';
+ });
+ return pos == scheme.end();
}
-std::tuple<std::string, uri::Error, uri::string_view_type>
parse_scheme(uri::string_arg_type uri) {
- auto pos = uri.find(':');
- if (pos == uri::npos) {
- return { "", uri::Error::InvalidScheme, uri };
- }
-
- auto scheme = uri.substr(0, pos);
- if (!::valid_scheme(scheme)) {
- return { "", uri::Error::InvalidScheme, uri };
- }
- std::string scheme_string{ scheme };
- std::transform(scheme_string.begin(), scheme_string.end(),
scheme_string.begin(),
- [](unsigned char c){ return std::tolower(c); });
-
- return { scheme_string, uri::Error::None, uri.substr(pos + 1) };
+std::tuple<std::string, uri::Error, uri::string_view_type> parse_scheme(
+ uri::string_arg_type uri) {
+ auto pos = uri.find(':');
+ if (pos == uri::npos) {
+ return {"", uri::Error::InvalidScheme, uri};
+ }
+
+ auto scheme = uri.substr(0, pos);
+ if (!::valid_scheme(scheme)) {
+ return {"", uri::Error::InvalidScheme, uri};
+ }
+ std::string scheme_string{scheme};
+ std::transform(scheme_string.begin(), scheme_string.end(),
+ scheme_string.begin(),
+ [](unsigned char c) { return std::tolower(c); });
+
+ return {scheme_string, uri::Error::None, uri.substr(pos + 1)};
}
-std::tuple<uri::Authority, uri::Error, uri::string_view_type>
parse_authority(uri::string_arg_type uri) {
- uri::Authority authority;
-
- bool has_authority = uri.length() >= 2 && uri[0] == '/' && uri[1] == '/';
- if (!has_authority) {
- return { authority, uri::Error::None, uri };
- }
-
- auto pos = uri.substr(2).find('/');
- auto auth_string = uri.substr(2, pos);
- auto rem = uri.substr(pos + 2);
- authority.authority = auth_string;
-
- pos = auth_string.find('@');
+std::tuple<uri::Authority, uri::Error, uri::string_view_type> parse_authority(
+ uri::string_arg_type uri) {
+ uri::Authority authority;
+
+ bool has_authority = uri.length() >= 2 && uri[0] == '/' && uri[1] == '/';
+ if (!has_authority) {
+ return {authority, uri::Error::None, uri};
+ }
+
+ auto pos = uri.substr(2).find('/');
+ auto auth_string = uri.substr(2, pos);
+ auto rem = uri.substr(pos + 2);
Review Comment:
parse_authority computes `pos = uri.substr(2).find('/')` and then
unconditionally does `rem = uri.substr(pos + 2)`. When no '/' exists (pos ==
npos), `pos + 2` wraps (size_t) and `rem` becomes an incorrect substring (e.g.,
`s3://bucket` yields path "/bucket" and later becomes "bucket/bucket"). Handle
the npos case explicitly by setting `rem` to an empty string_view/string and
taking `auth_string` as the remainder.
```suggestion
uri::string_view_type auth_string;
uri::string_view_type rem;
if (pos == uri::npos) {
auth_string = uri.substr(2);
rem = uri::string_view_type{};
} else {
auth_string = uri.substr(2, pos);
rem = uri.substr(pos + 2);
}
```
##########
.github/workflows/ci.yml:
##########
@@ -87,38 +87,15 @@ jobs:
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_TESTS=ON -DBUILD_EXAMPLES=ON
-DBUILD_BENCHMARKS=ON -DCMAKE_CXX_FLAGS="--coverage"
-DCMAKE_C_FLAGS="--coverage"
- - name: Cpp Format and lint
- working-directory: "cpp/build"
+ - name: clang-format
run: |
- # install clang-format
- sudo curl -L
https://github.com/muttleyxd/clang-tools-static-binaries/releases/download/master-22538c65/clang-format-8_linux-amd64
--output /usr/bin/clang-format
- sudo chmod +x /usr/bin/clang-format
-
- # validate format
- function prepend() { while read line; do echo "${1}${line}"; done; }
-
- make graphar-clformat
- GIT_DIFF=$(git diff --ignore-submodules)
- if [[ -n $GIT_DIFF ]]; then
- echo
"~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"
- echo "| clang-format failures found!"
- echo "|"
- echo "$GIT_DIFF" | prepend "| "
- echo "|"
- echo "| Run: "
- echo "|"
- echo "| make graphar-clformat"
- echo "|"
- echo "| to fix this error."
- echo "|"
- echo "| Ensure you are working with clang-format-8, which can be
obtained from"
- echo "|"
- echo "|
https://github.com/muttleyxd/clang-tools-static-binaries/releases";
- echo "|"
- echo
"~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~"
- exit -1
- fi
+ pip install pre-commit
+ pre-commit install
+ pre-commit run clang-format -a
Review Comment:
`pre-commit install` isn't needed in CI (it only installs git hooks) and can
be dropped to reduce side effects and runtime; running `pre-commit run
clang-format --all-files` directly is sufficient.
```suggestion
pre-commit run clang-format --all-files
```
##########
cpp/thirdparty/simple-uri-parser/uri_parser.h:
##########
@@ -27,215 +27,222 @@
#ifndef SIMPLE_URI_PARSER_LIBRARY_H
#define SIMPLE_URI_PARSER_LIBRARY_H
+#include <algorithm>
#include <string>
#include <unordered_map>
Review Comment:
This header uses std::string_view (when C++17+) and std::tuple/std::tie, but
it doesn't include <string_view> or <tuple>; it also calls
std::isalnum/std::tolower/std::strtol without including <cctype>/<cstdlib>. To
keep the header self-contained and portable across standard library
implementations, add the missing standard includes near the top.
```suggestion
#include <unordered_map>
#include <string_view>
#include <tuple>
#include <cctype>
#include <cstdlib>
```
##########
cpp/src/graphar/filesystem.cc:
##########
@@ -397,7 +397,6 @@ Status FinalizeS3() {
template Result<IdType> FileSystem::ReadFileToValue<IdType>(
const std::string&) const noexcept;
/// template specialization for std::string
-template Status FileSystem::WriteValueToFile<IdType>(const IdType&,
- const std::string&) const
- noexcept;
+template Status FileSystem::WriteValueToFile<IdType>(
+ const IdType&, const std::string&) const noexcept;
Review Comment:
These explicit template instantiation comments say "template specialization
for std::string" but the instantiations shown are for IdType
(ReadFileToValue<IdType> and WriteValueToFile<IdType>). Updating/removing the
misleading comments would avoid confusion when maintaining these explicit
instantiations.
##########
.pre-commit-config.yaml:
##########
@@ -39,7 +39,7 @@ repos:
rev: v21.1.2
hooks:
- id: clang-format
- files: ^python/
+ files: \.(cc|cpp|h|hpp)$
Review Comment:
The clang-format hook now matches all C++ extensions repo-wide, which also
pulls in vendored code under cpp/thirdparty (this repo appears to intentionally
exclude thirdparty from tooling, e.g., the CMake graphar-clformat target and
codecov ignore list). Consider excluding cpp/thirdparty (and any other
vendored/generated trees) via the hook's `exclude:` or by tightening the
`files:` regex, so formatting enforcement stays focused on maintained sources.
```suggestion
files: ^cpp/.*\.(cc|cpp|h|hpp)$
exclude: ^cpp/thirdparty/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
