feat: add windows support #119

dmehala · 2024-05-06T11:40:48Z

Description

This PR add support for windows. It is a prerequisite for #85.

Contains:

refactor: the codebase to use substr instead of range.
refactor: CMake targets.
refactor: add bazelrc to build using either abseil or std::string STD.
ci: build and run on windows using CMake and bazel.
update: add platform support section and update building instructions.

pr-commenter · 2024-05-06T11:54:07Z

Benchmarks

Benchmark execution time: 2024-05-20 21:14:39

Comparing candidate commit 7355dcc in PR branch dmehala/windows-support-2 with baseline commit f8cacf4 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

README.md

src/datadog/expected.h

test/system-tests/CMakeLists.txt

test/system-tests/main.cpp

pablomartinezbernardo

Small comment, LGTM

pablomartinezbernardo · 2024-05-16T14:16:15Z

src/datadog/string_util.cpp

 bool starts_with(StringView subject, StringView prefix) {
-  if (prefix.size() > subject.size()) {
-    return false;
+  auto s = subject.data();
+  auto p = prefix.data();
+  const auto prefix_end = p + prefix.size();
+  while (*s == *p) {
+    ++s;
+    ++p;
  }

-  return std::mismatch(subject.begin(), subject.end(), prefix.begin()).second ==
-         prefix.end();
+  return p == prefix_end;
 }


This will cause problems when subject is equal to prefix, right? Not that important, but should also not be difficult to fix

[question] why?

There's nothing stopping s and p from going over the end of the string_view, so it would be undefined behavior, but even if nothing breaks the condition p == prefix_end will be false. Did a small example to reproduce it

To avoid that, the function could be written like this for example

bool starts_with(StringView subject, StringView prefix) { auto s = subject.data(); auto p = prefix.data(); const auto prefix_end = p + prefix.size(); while (p < prefix_end) { if (*s++ != *p++) { return false; } } return true; }

There's nothing stopping s and p from going over the end of the string_view, so it would be undefined behavior

Indeed, thanks to Github awful diff I thought if (prefix.size() > subject.size()) { ... } was part of the implementation, which reduce the area of the UB. However, as you mentioned, if prefix.size() == subject.size() then it's undefined behaviour and plain wrong. Thank you.

Fix implementation + add unit tests.

- refactor: the codebase to use `substr` instead of `range`. - refactor: CMake targets. - refactor: add bazelrc to build using either abseil or std::string STD. - ci: build and run on windows using CMake and bazel. - update: add platform support section and update building instructions.

src/datadog/platform_util.cpp

dmehala force-pushed the dmehala/windows-support-2 branch from e98f1b7 to a98849c Compare May 6, 2024 11:51

dmehala marked this pull request as ready for review May 6, 2024 11:57