[WIP] Feature improve exception message details (closes #184) #667

coatless · 2017-04-10T17:27:14Z

Overview

This is a work in progress (WIP) PR to upgrade the exception messages found in Rcpp.

For details on the contents of the PR, please see #184. (The issue ticket contains GitHub search URL for each Rcpp exception message.)

Changes:

Added RCPP_ADVANCED_EXCEPTION_CLASS macro that mimics the structure of the Rcpp::stop() / Rcpp::warning() implementations.
Removed no_such_field
Changed throw type from not_compatible to not_a_matrix in inst\include\Rcpp\vector\Matrix.h (FYI, a throw was missing on one of the statements).

Presently, I've held off on removing reference_creation_error as it is not used but it is referenced within the documentation.

Misc

This branch will likely need to be rebased after @jimhester's #663 PR.

codecov-io · 2017-04-10T17:48:35Z

Codecov Report

Merging #667 into master will decrease coverage by 3.08%.
The diff coverage is 39.28%.

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
- Coverage   92.91%   89.83%   -3.09%     
==========================================
  Files          65       65              
  Lines        3303     3511     +208     
==========================================
+ Hits         3069     3154      +85     
- Misses        234      357     +123

Impacted Files	Coverage Δ
inst/include/Rcpp/r_cast.h	`100% <ø> (ø)`	⬆️
inst/include/Rcpp/S4.h	`70.58% <ø> (ø)`	⬆️
inst/include/Rcpp/proxy/SlotProxy.h	`91.66% <0%> (ø)`	⬆️
inst/include/Rcpp/Function.h	`80% <0%> (-8.89%)`	⬇️
inst/include/Rcpp/XPtr.h	`60.52% <25%> (-1.64%)`	⬇️
inst/include/Rcpp/internal/export.h	`87.5% <33.33%> (-12.5%)`	⬇️
inst/include/Rcpp/as.h	`79.31% <33.33%> (-12.69%)`	⬇️
inst/include/Rcpp/vector/Vector.h	`86.82% <66.66%> (-1.09%)`	⬇️
inst/include/Rcpp/utils/tinyformat.h	`42.34% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 886f5df...d8c1dc3. Read the comment docs.

kevinushey · 2017-04-10T17:50:18Z

inst/include/Rcpp/api/meat/DottedPairImpl.h

@@ -53,7 +53,7 @@ namespace Rcpp{
 		} else {
 			if( ref.isNULL( ) ) throw index_out_of_bounds() ;

-                        if( static_cast<R_xlen_t>(index) > ::Rf_xlength(ref.get__()) ) throw index_out_of_bounds() ;
+			if( static_cast<R_xlen_t>(index) > ::Rf_xlength(ref.get__()) ) throw index_out_of_bounds("index is out of bounds. Requested %i with %i available", static_cast<R_xlen_t>(index), ::Rf_xlength(ref.get__()) ) ;


Minor, but to avoid overly long lines like this I often use an idiom like (pseudo-ish code):

if (condition) { const char* fmt = "Index out of bounds: [index=%s; extent=%s]" throw index_out_of_bounds(fmt, index, length); }

And once can wrap the "...." over two lines as "abc" \n "def".

Luckily we have no strict 80 column policy here :)

Aye, that makes it a lot more manageable. I'll switch everything over.

Pushed through a commit that addressed this feedback.

kevinushey · 2017-04-10T17:54:11Z

inst/include/Rcpp/exceptions.h

@@ -89,6 +89,48 @@ namespace Rcpp {
            file_io_error("file already exists", file) {}	// #nocov end
    };

+    #define RCPP_ADVANCED_EXCEPTION_CLASS(__CLASS__, __WHAT__)                                                                                                                                                     \


Minor suggestion, but it'd be nice if we had a C++11 version (using variadic templates) and a C++98 version (using your current code-bloat version). We could do something like:

#if __cplusplus >= 201103L # include <Rcpp/exceptions/cpp11/exceptions.h> #else # include <Rcpp/exceptions/cpp98/exceptions.h> #endif

Let me know if you think it's worth the effort.

It would be very nice to have Vardiac template support... Mmm.

Would something along the lines of:

#define RCPP_CPP11_EXCEPTION_CLASS(__CLASS__, __WHAT__) class __CLASS__ : public std::exception{ \ public: \ __CLASS__( const std::string& message ) throw() : message( __WHAT__ ){} ; \ template <typename... Args> \ __CLASS__( const char* fmt, Args&&... args ) throw() : \ message( tfm::format(fmt, std::forward<Args>(args)...) ){} ; \ virtual ~__CLASS__() throw(){} ; \ virtual const char* what() const throw() { return message.c_str() ; } \ private: \ std::string message ; \ } ;

Be acceptable?

Note: The defines in /inst/include/Rcpp/utils/tinyformat.h will also need to receive the if __cplusplus >= 201103L check.

Yes, something exactly like that is what I was thinking of :)

Indeed, I think we somewhat bluntly said "make tinyformat work in C++98 mode" a while back without consideration for C++11 mode, so it would make sense to guard the define there according to whether we're using C++98 or >C++98.

We have some years-old tests for macros for C++11 which can probably do with brush-up. Now that we will have a decent compiler "most of the time" we may as well use it.

But we can of course not loose sight of the poor souls clinging to their old RHEL/CentOS/... systems from the last ice age. It's is probably worth our while to ensure those build too.

Enabling C++11 mode for tinyformat triggered a lot of error spam that is stuttering shell. So, I've left out this change until I can investigate the file a bit more after I grab some sleep.

I think we may have tried that too in the past and retreated. Foggy memory on this though...

I'm going to push an updated version of the tinyformat.h. It last received love in Aug 29, 2015. I need a fix released in March 2016 to quiet a [-Wuninitialized] complaint. After that, everything else should work.

Sure.

But let's open an issue ticket, preferably with a link to the tinyformat repo and changelog, and take this out of this somewhat growing discussion.

this example still triggers messages from R-devel CMD check after discussion with jmc, concluded that these may stem from the R side

Added support for throwing exceptions without call stacks.

commit 43e53b0 Merge: 886f5df a15ba45 Author: Dirk Eddelbuettel <edd@debian.org> Date: Tue Apr 11 13:20:10 2017 -0500 Merge pull request #663 from jimhester/exceptions_without_callstacks Added support for throwing exceptions without call stacks. commit a15ba45 Author: Jim Hester <james.f.hester@gmail.com> Date: Mon Apr 3 16:11:24 2017 -0400 Added support for throwing exceptions without call stacks.

Feature/update modules example

…ges' into feature/improved-exception-messages

jimhester and others added 3 commits April 3, 2017 16:23

Added support for throwing exceptions without call stacks.

a15ba45

updates to testRcppModule

63fb5a0

Improve exception message details

fd063ef

coatless changed the title ~~[WIP] Feature improve exception message details~~ [WIP] Feature improve exception message details (closes #184) Apr 10, 2017

kevinushey reviewed Apr 10, 2017

View reviewed changes

eddelbuettel and others added 10 commits April 11, 2017 13:06

updates to testRcppClass

a8dbfa1

this example still triggers messages from R-devel CMD check after discussion with jmc, concluded that these may stem from the R side

Merge pull request #663 from jimhester/exceptions_without_callstacks

43e53b0

Added support for throwing exceptions without call stacks.

Merge branch 'master' into feature/update_modules_example

12cabff

Merge pull request #668 from RcppCore/feature/update_modules_example

21d8388

Feature/update modules example

Improve exception message details

a67d16a

Revised exception messages

ba4403d

Merge remote-tracking branch 'origin/feature/improved-exception-messa…

d8c1dc3

…ges' into feature/improved-exception-messages

Refreshed tinyformat.h against upstream, retained local mods

ec16932

Passed MIA S4 slot name to exception throw

3326d70

coatless mentioned this pull request Apr 14, 2017

Refresh tinyformat.h against upstream to quiet -Wuninitialized with C++11 on clang #673

Closed

Call exception builders dependent on C++11 or C++98

41cad49

coatless mentioned this pull request Apr 15, 2017

better error messages #184

Closed

coatless closed this Apr 17, 2017

coatless deleted the feature/improved-exception-messages branch August 16, 2017 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[WIP] Feature improve exception message details (closes #184) #667

[WIP] Feature improve exception message details (closes #184) #667

Uh oh!

coatless commented Apr 10, 2017

Uh oh!

codecov-io commented Apr 10, 2017 •

edited

Loading

Uh oh!

kevinushey Apr 10, 2017

Uh oh!

eddelbuettel Apr 10, 2017

Uh oh!

coatless Apr 10, 2017

Uh oh!

coatless Apr 14, 2017

Uh oh!

kevinushey Apr 10, 2017

Uh oh!

coatless Apr 10, 2017

Uh oh!

kevinushey Apr 11, 2017 •

edited

Loading

Uh oh!

eddelbuettel Apr 11, 2017

Uh oh!

coatless Apr 14, 2017

Uh oh!

eddelbuettel Apr 14, 2017

Uh oh!

coatless Apr 14, 2017

Uh oh!

eddelbuettel Apr 14, 2017

Uh oh!

Uh oh!

Uh oh!

[WIP] Feature improve exception message details (closes #184) #667

[WIP] Feature improve exception message details (closes #184) #667

Uh oh!

Conversation

coatless commented Apr 10, 2017

Overview

Changes:

Misc

Uh oh!

codecov-io commented Apr 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinushey Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Apr 10, 2017 •

edited

Loading

kevinushey Apr 11, 2017 •

edited

Loading