You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/11/07 17:59:08 UTC

[GitHub] [arrow] pitrou opened a new pull request, #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

pitrou opened a new pull request, #14602:
URL: https://github.com/apache/arrow/pull/14602

   We can use std::regex now that we needn't support gcc 4.9 anymore.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1306019797

   @github-actions crossbow submit -g python


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1020921801


##########
cpp/src/arrow/util/regex.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cassert>
+#include <initializer_list>
+#include <regex>
+#include <string_view>
+#include <type_traits>
+
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+namespace internal {
+
+/// Match regex against target and produce string_views out of matches.
+bool RegexMatch(const std::regex& regex, std::string_view target,

Review Comment:
   You're right, will do.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] bkietz commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
bkietz commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1019620241


##########
cpp/src/arrow/util/regex.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cassert>
+#include <initializer_list>
+#include <regex>
+#include <string_view>
+#include <type_traits>
+
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+namespace internal {
+
+/// Match regex against target and produce string_views out of matches.
+bool RegexMatch(const std::regex& regex, std::string_view target,

Review Comment:
   This needs to be marked inline if it's going to be defined in a header; otherwise we'll get link errors when this gets included more places.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] github-actions[bot] commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1306019441

   ```
   No such option: -g
   The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/3413002548
   ```


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1020931879


##########
cpp/src/arrow/util/regex.h:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cassert>
+#include <initializer_list>
+#include <regex>
+#include <string_view>
+#include <type_traits>
+
+#include "arrow/util/visibility.h"
+
+namespace arrow {
+namespace internal {
+
+/// Match regex against target and produce string_views out of matches.
+bool RegexMatch(const std::regex& regex, std::string_view target,

Review Comment:
   PR at https://github.com/apache/arrow/pull/14626



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] github-actions[bot] commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1306023269

   Revision: 070eda9828e58293fb0e0742c90df333e4a21631
   
   Submitted crossbow builds: [ursacomputing/crossbow @ actions-21ac1af44b](https://github.com/ursacomputing/crossbow/branches/all?query=actions-21ac1af44b)
   
   |Task|Status|
   |----|------|
   |test-conda-python-3.10|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.10)](https://github.com/ursacomputing/crossbow/actions/runs/3413019618/jobs/5679246742)|
   |test-conda-python-3.11|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.11)](https://github.com/ursacomputing/crossbow/actions/runs/3413020847/jobs/5679249566)|
   |test-conda-python-3.7|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7)](https://github.com/ursacomputing/crossbow/actions/runs/3413023364/jobs/5679255711)|
   |test-conda-python-3.7-hdfs-2.9.2|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7-hdfs-2.9.2)](https://github.com/ursacomputing/crossbow/actions/runs/3413021250/jobs/5679250581)|
   |test-conda-python-3.7-hdfs-3.2.1|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7-hdfs-3.2.1)](https://github.com/ursacomputing/crossbow/actions/runs/3413021769/jobs/5679251762)|
   |test-conda-python-3.7-pandas-0.24|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7-pandas-0.24)](https://github.com/ursacomputing/crossbow/actions/runs/3413026347/jobs/5679262268)|
   |test-conda-python-3.7-pandas-latest|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7-pandas-latest)](https://github.com/ursacomputing/crossbow/actions/runs/3413022368/jobs/5679253212)|
   |test-conda-python-3.7-spark-v3.1.2|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.7-spark-v3.1.2)](https://github.com/ursacomputing/crossbow/actions/runs/3413027370/jobs/5679264668)|
   |test-conda-python-3.8|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.8)](https://github.com/ursacomputing/crossbow/actions/runs/3413022751/jobs/5679254224)|
   |test-conda-python-3.8-hypothesis|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.8-hypothesis)](https://github.com/ursacomputing/crossbow/actions/runs/3413025999/jobs/5679261459)|
   |test-conda-python-3.8-pandas-latest|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.8-pandas-latest)](https://github.com/ursacomputing/crossbow/actions/runs/3413023013/jobs/5679254947)|
   |test-conda-python-3.8-pandas-nightly|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.8-pandas-nightly)](https://github.com/ursacomputing/crossbow/actions/runs/3413020397/jobs/5679248447)|
   |test-conda-python-3.8-spark-v3.2.0|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.8-spark-v3.2.0)](https://github.com/ursacomputing/crossbow/actions/runs/3413025109/jobs/5679259631)|
   |test-conda-python-3.9|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.9)](https://github.com/ursacomputing/crossbow/actions/runs/3413023850/jobs/5679256884)|
   |test-conda-python-3.9-dask-latest|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.9-dask-latest)](https://github.com/ursacomputing/crossbow/actions/runs/3413025759/jobs/5679260944)|
   |test-conda-python-3.9-dask-upstream_devel|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.9-dask-upstream_devel)](https://github.com/ursacomputing/crossbow/actions/runs/3413019091/jobs/5679245868)|
   |test-conda-python-3.9-pandas-upstream_devel|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.9-pandas-upstream_devel)](https://github.com/ursacomputing/crossbow/actions/runs/3413026740/jobs/5679263265)|
   |test-conda-python-3.9-spark-master|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-conda-python-3.9-spark-master)](https://github.com/ursacomputing/crossbow/actions/runs/3413027017/jobs/5679263870)|
   |test-cuda-python|[![Github Actions](https://github.com/ursacomputing/crossbow/workflows/Crossbow/badge.svg?branch=actions-21ac1af44b-github-test-cuda-python)](https://github.com/ursacomputing/crossbow/actions/runs/3413024682/jobs/5679258666)|
   |test-debian-11-python-3|[![Azure](https://dev.azure.com/ursacomputing/crossbow/_apis/build/status/ursacomputing.crossbow?branchName=actions-21ac1af44b-azure-test-debian-11-python-3)](https://github.com/ursacomputing/crossbow/runs/9343219466)|
   |test-fedora-35-python-3|[![Azure](https://dev.azure.com/ursacomputing/crossbow/_apis/build/status/ursacomputing.crossbow?branchName=actions-21ac1af44b-azure-test-fedora-35-python-3)](https://github.com/ursacomputing/crossbow/runs/9343227246)|
   |test-ubuntu-20.04-python-3|[![Azure](https://dev.azure.com/ursacomputing/crossbow/_apis/build/status/ursacomputing.crossbow?branchName=actions-21ac1af44b-azure-test-ubuntu-20.04-python-3)](https://github.com/ursacomputing/crossbow/runs/9343232111)|


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1310356377

   Will merge if CI passes.


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] ursabot commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1311182662

   Benchmark runs are scheduled for baseline = 18326f96834169b4573f3d4f1c0d46f1e8499295 and contender = f5767179cb4a4d8c33a7010ebcf52aee693a4159. f5767179cb4a4d8c33a7010ebcf52aee693a4159 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
   Conbench compare runs links:
   [Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/ae64599302844756b69e0ff03ff459b0...e4ec40e727944cf3a2775ef058da78c7/)
   [Finished :arrow_down:1.12% :arrow_up:0.03%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/566026a5bf44429dba22e9d25e28946f...201e4ccf1991461cbc35bd164454d92d/)
   [Finished :arrow_down:0.54% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ed762c0b680c43caa4de9a99cc5df2ea...b4670f10c20c4e949f1f07131a08763a/)
   [Finished :arrow_down:1.55% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/1dc62010b39d4a4badbcc9311d908533...d773e28b0601439ab8d289d65f13a59c/)
   Buildkite builds:
   [Finished] [`f5767179` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1850)
   [Finished] [`f5767179` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1873)
   [Finished] [`f5767179` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1840)
   [Finished] [`f5767179` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1865)
   [Finished] [`18326f96` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1849)
   [Finished] [`18326f96` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1871)
   [Finished] [`18326f96` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1839)
   [Finished] [`18326f96` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1864)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
   test-mac-arm: Supported benchmark langs: C++, Python, R
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] github-actions[bot] commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1305983929

   https://issues.apache.org/jira/browse/ARROW-18270


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1015737637


##########
python/pyarrow/src/arrow/python/datetime.cc:
##########
@@ -38,41 +39,27 @@ namespace internal {
 
 namespace {
 
-// Same as Regex '([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$'.
-// GCC 4.9 doesn't support regex, so handcode until support for it
-// is dropped.
 bool MatchFixedOffset(const std::string& tz, std::string_view* sign,
                       std::string_view* hour, std::string_view* minute) {
+  static const std::regex regex("^([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$");
   if (tz.size() < 5) {
     return false;
   }
-  const char* iter = tz.data();
-  if (*iter == '+' || *iter == '-') {
-    *sign = std::string_view(iter, 1);
-    iter++;
-    if (tz.size() < 6) {
-      return false;
-    }
-  }
-  if ((((*iter == '0' || *iter == '1') && *(iter + 1) >= '0' && *(iter + 1) <= '9') ||
-       (*iter == '2' && *(iter + 1) >= '0' && *(iter + 1) <= '3'))) {
-    *hour = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  if (*iter != ':') {
+  std::smatch match;
+  if (!std::regex_match(tz, match, regex)) {
     return false;
   }
-  iter++;
+  DCHECK_EQ(match.size(), 4);  // match #0 is the whole matched sequence
 
-  if (*iter >= '0' && *iter <= '5' && *(iter + 1) >= '0' && *(iter + 1) <= '9') {
-    *minute = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  return iter == (tz.data() + tz.size());
+  auto match_view = [&](int match_number) {
+    return std::string_view(&tz[match.position(match_number)],
+                            match.length(match_number));
+  };

Review Comment:
   @bkietz Do you know if there's a more elegant way to write this? (get a string view from a regex match)



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] bkietz commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
bkietz commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1018189597


##########
python/pyarrow/src/arrow/python/datetime.cc:
##########
@@ -38,41 +39,27 @@ namespace internal {
 
 namespace {
 
-// Same as Regex '([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$'.
-// GCC 4.9 doesn't support regex, so handcode until support for it
-// is dropped.
 bool MatchFixedOffset(const std::string& tz, std::string_view* sign,
                       std::string_view* hour, std::string_view* minute) {
+  static const std::regex regex("^([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$");
   if (tz.size() < 5) {
     return false;
   }
-  const char* iter = tz.data();
-  if (*iter == '+' || *iter == '-') {
-    *sign = std::string_view(iter, 1);
-    iter++;
-    if (tz.size() < 6) {
-      return false;
-    }
-  }
-  if ((((*iter == '0' || *iter == '1') && *(iter + 1) >= '0' && *(iter + 1) <= '9') ||
-       (*iter == '2' && *(iter + 1) >= '0' && *(iter + 1) <= '3'))) {
-    *hour = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  if (*iter != ':') {
+  std::smatch match;
+  if (!std::regex_match(tz, match, regex)) {
     return false;
   }
-  iter++;
+  DCHECK_EQ(match.size(), 4);  // match #0 is the whole matched sequence
 
-  if (*iter >= '0' && *iter <= '5' && *(iter + 1) >= '0' && *(iter + 1) <= '9') {
-    *minute = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  return iter == (tz.data() + tz.size());
+  auto match_view = [&](int match_number) {
+    return std::string_view(&tz[match.position(match_number)],
+                            match.length(match_number));
+  };

Review Comment:
   I can't improve on this by much. `std::regex` really expects you to want a string.
   
   ```c++
   bool MatchFixedOffset(std::string_view tz, std::string_view* sign,
                         std::string_view* hour, std::string_view* minute) {
     static const std::regex regex("^([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$");
     if (tz.size() < 5) {
       return false;
     }
   
     std::match_results<decltype(target.begin())> match;
     if (!std::regex_match(tz.begin(), tz.end(), match, regex)) {
       return false;
     }
     DCHECK_EQ(match.size(), 4);  // match #0 is the whole matched sequence
   
     auto match_view = [&](int i) {
       return tz.substr(match.position(i), match.length(i));
     };
     *sign = match_view(1);
     *hour = match_view(2);
     *minute = match_view(3);
     return true;
   }
   ```
   
   OTOH, this might be a common enough problem to make a generic helper function for:
   
   ```c++
   // in util/string.h or util/regex.h
   bool Match(const std::regex& regex, std::string_view target,
                      std::initializer_list<std::string_view*> out_views) {
     std::match_results<decltype(target.begin())> match;
     if (!std::regex_match(target.begin(), target.end(), match, regex)) {
       return false;
     }
     DCHECK_EQ(match.size(), out_views.size() + 1); // match #0 is the whole matched sequence
   
     size_t i = 1;
     for (std::string_view* out_view : out_views) {
       *out_view = target.substr(match.position(i), match.length(i));
       ++i;
     }
     return true;
   }
   
   bool MatchFixedOffset(const std::string& tz, std::string_view* sign,
                         std::string_view* hour, std::string_view* minute) {
     if (tz.size() < 5) {
       return false;
     }
     static const std::regex regex("^([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$");
     return Match(tz, regex, {sign, hour, minute});
   }
   ```



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou merged pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou merged PR #14602:
URL: https://github.com/apache/arrow/pull/14602


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on a diff in pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #14602:
URL: https://github.com/apache/arrow/pull/14602#discussion_r1018961924


##########
python/pyarrow/src/arrow/python/datetime.cc:
##########
@@ -38,41 +39,27 @@ namespace internal {
 
 namespace {
 
-// Same as Regex '([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$'.
-// GCC 4.9 doesn't support regex, so handcode until support for it
-// is dropped.
 bool MatchFixedOffset(const std::string& tz, std::string_view* sign,
                       std::string_view* hour, std::string_view* minute) {
+  static const std::regex regex("^([+-])(0[0-9]|1[0-9]|2[0-3]):([0-5][0-9])$");
   if (tz.size() < 5) {
     return false;
   }
-  const char* iter = tz.data();
-  if (*iter == '+' || *iter == '-') {
-    *sign = std::string_view(iter, 1);
-    iter++;
-    if (tz.size() < 6) {
-      return false;
-    }
-  }
-  if ((((*iter == '0' || *iter == '1') && *(iter + 1) >= '0' && *(iter + 1) <= '9') ||
-       (*iter == '2' && *(iter + 1) >= '0' && *(iter + 1) <= '3'))) {
-    *hour = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  if (*iter != ':') {
+  std::smatch match;
+  if (!std::regex_match(tz, match, regex)) {
     return false;
   }
-  iter++;
+  DCHECK_EQ(match.size(), 4);  // match #0 is the whole matched sequence
 
-  if (*iter >= '0' && *iter <= '5' && *(iter + 1) >= '0' && *(iter + 1) <= '9') {
-    *minute = std::string_view(iter, 2);
-    iter += 2;
-  } else {
-    return false;
-  }
-  return iter == (tz.data() + tz.size());
+  auto match_view = [&](int match_number) {
+    return std::string_view(&tz[match.position(match_number)],
+                            match.length(match_number));
+  };

Review Comment:
   That helper is a nice idea, thanks.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [arrow] pitrou commented on pull request #14602: ARROW-18270: [Python] Remove gcc 4.9 compatibility code

Posted by GitBox <gi...@apache.org>.
pitrou commented on PR #14602:
URL: https://github.com/apache/arrow/pull/14602#issuecomment-1306018952

   @github-actions crossbow -g python


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org