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

Explicitly call abs as a function in DataFrame.h #1069

Merged
merged 1 commit into from
Apr 15, 2020

Conversation

xhochy
Copy link
Contributor

@xhochy xhochy commented Apr 15, 2020

When building with clang (in this case on Windows), the call to abs is expanded to the vectorised form although we call it with a scalar value. Explicitly requesting std::abs prohibits this expansion.

For reference, the original error:

/C/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Library/bin/clang++.exe  -std=gnu++14 -I"C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/lib/R/include" -D
NDEBUG -DNDEBUG -I"%BUILD_PREFIX%\Library/include" -I"%BUILD_PREFIX%/include" -O2 -D_CRT_SECURE_NO_WARNINGS -DARROW_R_WITH_ARROW -fuse-ld=lld -I"C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848
759312/_h_env/Lib/R/library/Rcpp/include"   -I"C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/lib/R/../../Library/mingw-w64/include"     -O2 -Wall -gdwarf-2 -march=x86-64 -mtune=
generic -c recordbatch.cpp -o recordbatch.o
In file included from recordbatch.cpp:18:
In file included from ././arrow_types.h:145:
In file included from C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp.h:57:
C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp/DataFrame.h:82:24: error: no matching function for call to 'abs'
                return abs(INTEGER(rn)[1]);
                       ^~~
recordbatch.cpp:100:47: note: in instantiation of member function 'Rcpp::DataFrame_Impl<PreserveStorage>::nrow' requested here
  return arrow::RecordBatch::Make(schema, tbl.nrow(), std::move(arrays));
                                              ^
C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp/sugar/functions/math.h:42:19: note: candidate function not viable: no known conversion from 'int'
      to 'SEXP' (aka 'SEXPREC *') for 1st argument
VECTORIZED_MATH_1(abs,::fabs)
                  ^
C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp/sugar/block/Vectorized_Math.h:91:9: note: expanded from macro 'VECTORIZED_MATH_1'
        __NAME__( SEXP x){ return __NAME__( NumericVector( x ) ) ; }             \
        ^
C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp/sugar/functions/math.h:42:19: note: candidate template ignored: could not match
      'VectorBase<14, NA, type-parameter-0-1>' against 'int'
C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/Lib/R/library/Rcpp/include\Rcpp/sugar/functions/math.h:42:19: note: candidate template ignored: could not match
      'VectorBase<13, NA, type-parameter-0-1>' against 'int'
1 error generated.
make: *** [recordbatch.o] Error 1C:/Users/Administrator/miniconda3/conda-bld/r-arrow_1586848759312/_h_env/lib/R/etc/x64/Makeconf:215: recipe for target 'recordbatch.o' failed

Checklist

  • Code compiles correctly
  • R CMD check still passes all tests
  • Prefereably, new tests were added which fail without the change
  • Document the changes by file in ChangeLog

Copy link
Member

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looks a valid catch. We generally tried to be explicit but in a large enough code base...

@eddelbuettel
Copy link
Member

Out of curiousity where/why/how would you build with clang on Windoze, and how is the cooperation with Rtools going?

@xhochy
Copy link
Contributor Author

xhochy commented Apr 15, 2020

Out of curiousity where/why/how would you build with clang on Windoze, and how is the cooperation with Rtools going?

I'm trying to the Apache Arrow libraries on Windows for R and Python with using the same Arrow C++ arrow.dll. This has been quite adventurous as they use different toolchains (MSVC for the Python world and MinGW in the RTools setting). By building the C++ part of the R Arrow package with clang, I was able to link (using lld) against the MSVC-based arrow.dll and the MinGW-based R world. Tests run smoothly for now. I'll try to write a blog post if this really works stable.

@eddelbuettel
Copy link
Member

I tried / had to try similar gymnastics in the past when shepherding projects at workplaces relying on vendor-supplied DLLs. It is generally ... a nightmare and I am quite content to not work on Windows that much anymore. And Python/R interop always was a huge challenge there for the reason you nailed down: two different toolchains. If you were to reliably bridge that it would be a huge deal.

The official Rcpp statement on this is in the Rcpp FAQ and could be shortened to don't bother ... But keep us posted.

@eddelbuettel eddelbuettel merged commit a80908f into RcppCore:master Apr 15, 2020
@xhochy xhochy deleted the clang branch April 15, 2020 13:51
@eddelbuettel
Copy link
Member

BTW @xhochy the old (Emacs et al standard) ChangeLog format has two spaces around the name:

modified   ChangeLog
@@ -1,4 +1,4 @@
-2020-04-15 Uwe Korn <mail@uwekorn.com>
+2020-04-15  Uwe Korn  <mail@uwekorn.com>
 
 	* inst/include/Rcpp/DataFrame.h: Explicit call to scalar std::abs

Just in case you ever feel compelled to send another PR with a ChangeLog entry. Appreciate you put one in. Greetings from an ex-Karlsruher...

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.

None yet

2 participants