jonkeane commented on a change in pull request #11404:
URL: https://github.com/apache/arrow/pull/11404#discussion_r729776246
##########
File path: r/src/array_to_vector.cpp
##########
@@ -786,13 +786,7 @@ class Converter_Struct : public Converter {
private:
std::vector<std::shared_ptr<Converter>> converters;
- bool is_altrep(SEXP x) const {
-#if defined(HAS_ALTREP)
- return ALTREP(x);
-#else
- return false;
-#endif
- }
+ bool is_altrep(SEXP x) const { return ALTREP(x); }
Review comment:
I was worried about that too, but then I saw in altrep.hpp:
```
#if defined(R_VERSION) && R_VERSION >= R_Version(3, 5, 0)
#define HAS_ALTREP
#endif
#ifndef HAS_ALTREP
#define ALTREP(x) false
...
```
If I'm reading that correctly `ALTREP` will return false before 3.6.0. The
tests for prior versions also seemed to pass (though you would have a better
idea if we are already exercising this macro in our R tests in earlier versions
or if we have gated the test enough that we would never hit it in the test, but
folks would in real life). I'm happy to add it back, of course
--
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]
