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

Replacing const char* arguments with std::string in error functions #584

Merged
merged 8 commits into from Aug 3, 2017

Conversation

Projects
None yet
3 participants
@syclik
Member

syclik commented Jul 29, 2017

Submission Checklist

  • Run unit tests: ./runTests.py test/unit
  • Run cpplint: make cpplint
  • Declare copyright holder and open-source license: see below

Summary:

Cleaning up code by replacing uses of const char* with const std::string&.

This shouldn't affect the behavior of the code.

Intended Effect:

Make our code base more idiomatic C++ and more consistent.

How to Verify:

Check the diff. Most of it is going to be very repetitive.

Side Effects:

None.

Documentation:

None.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):

Stan Group

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

@syclik syclik requested review from seantalts and bob-carpenter Jul 29, 2017

@seantalts

Looks good minus the typo and maybe accidental removal of a (%1%)(?)

I don't know much about the design tradeoffs between char * and std::string but from what I briefly read this seems like a reasonable thing to do.

Show outdated Hide outdated stan/math/prim/scal/prob/normal_sufficient_lpdf.hpp
@@ -73,7 +73,7 @@ namespace stan {
typename return_type<T_y, T_alpha, T_tau, T_beta, T_delta>::type
wiener_lpdf(const T_y& y, const T_alpha& alpha, const T_tau& tau,
const T_beta& beta, const T_delta& delta) {
static const char* function("wiener_lpdf(%1%)");
static const std::string function = "wiener_lpdf";

This comment has been minimized.

@seantalts

seantalts Jul 31, 2017

Member

Was this a mistake before and it's fixed now? I see some other (%1%) in the code; are they used as sort of format strings?

@seantalts

seantalts Jul 31, 2017

Member

Was this a mistake before and it's fixed now? I see some other (%1%) in the code; are they used as sort of format strings?

@bob-carpenter

This comment has been minimized.

Show comment
Hide comment
@bob-carpenter

bob-carpenter Jul 31, 2017

Contributor

@seantalts Yes, we definitely want to make this change.

I'm pretty sure all those %1% things are holdovers from back when we were using boost formatting that just got code transformed along with everything else. We stopped using Boost formatting because it has the same awful runtime error behavior as printf.

Contributor

bob-carpenter commented Jul 31, 2017

@seantalts Yes, we definitely want to make this change.

I'm pretty sure all those %1% things are holdovers from back when we were using boost formatting that just got code transformed along with everything else. We stopped using Boost formatting because it has the same awful runtime error behavior as printf.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Aug 1, 2017

Member

+1 to Bob's comment.

The %1% was from when we used boost's formatting for output messages (when we had boost's policies in place too). I thought we had removed it, but I found a dangling instance, so cleaned it up in the process.

Member

syclik commented Aug 1, 2017

+1 to Bob's comment.

The %1% was from when we used boost's formatting for output messages (when we had boost's policies in place too). I thought we had removed it, but I found a dangling instance, so cleaned it up in the process.

@syclik

This comment has been minimized.

Show comment
Hide comment
@syclik

syclik Aug 1, 2017

Member

@seantalts thanks for picking up on that accidentally deleted paren. Fixed, pushed. Waiting on tests now.

Member

syclik commented Aug 1, 2017

@seantalts thanks for picking up on that accidentally deleted paren. Fixed, pushed. Waiting on tests now.

@syclik syclik merged commit 6fc8e2a into develop Aug 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@syclik syclik deleted the feature/issue-460-const-char-star branch Aug 3, 2017

seantalts added a commit that referenced this pull request Nov 1, 2017

Revert "Merge pull request #584 from stan-dev/feature/issue-460-const…
…-char-star"

This reverts commit 6fc8e2a, reversing
changes made to 5ed2d69.

seantalts added a commit that referenced this pull request Nov 3, 2017

seantalts added a commit that referenced this pull request Nov 3, 2017

seantalts added a commit that referenced this pull request Nov 3, 2017

seantalts added a commit that referenced this pull request Nov 3, 2017

@seantalts seantalts referenced this pull request Nov 3, 2017

Merged

Feature/Normal_Id_GLM #665

3 of 3 tasks complete

seantalts added a commit that referenced this pull request Nov 9, 2017

seantalts added a commit that referenced this pull request Nov 10, 2017

yizhang-cae added a commit to yizhang-cae/math that referenced this pull request Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment