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

Cast std::max args to Dtype #3320

Merged
merged 1 commit into from
Nov 27, 2015
Merged

Cast std::max args to Dtype #3320

merged 1 commit into from
Nov 27, 2015

Conversation

eelstork
Copy link
Contributor

This is to avoid ambiguous literals. Consistent with similar idioms in Caffe source. Fixes errors with MSVC compiler.

@eelstork eelstork changed the title Cast number literals to Dtype Cast std literals to Dtype Nov 12, 2015
@eelstork eelstork changed the title Cast std literals to Dtype Cast std::max args to Dtype Nov 12, 2015
@seanbell
Copy link

You can achieve the same effect more cleanly with std::max<Dtype>(a, b) instead of std::max(Dtype(a), Dtype(a)).

@eelstork
Copy link
Contributor Author

@seanbell I am aware of this (@willyd and others fixed it this way). Is it cleaner, or only more concise?

@seanbell
Copy link

I personally think it's more clear/clean, since max<T> is saying "use the max function for type T." I started using it after I ran into the problem that abs can sometimes lead to unintentional casting to integers (I needed fabs), but abs<T> would either do the right thing or generate an error (depending on which abs definition is included). We're talking about a different function (max), but that's why I like explicitly specifying which template is used via function<T>.

Anyway, that's just my opinion. Both versions work.

@longjon
Copy link
Contributor

longjon commented Nov 15, 2015

I agree with @seanbell here... in addition, it's best to avoid C-style casting (I can see an argument being made for literals, but we are not only casting literals here).

@shelhamer
Copy link
Member

Agreed with @seanbell and @longjon. Please revise for merge. Thanks for the the compatibility check.

@eelstork
Copy link
Contributor Author

Updated to use suggested form.

ronghanghu added a commit that referenced this pull request Nov 27, 2015
@ronghanghu ronghanghu merged commit 4e9b449 into BVLC:master Nov 27, 2015
@ronghanghu
Copy link
Member

Thanks @eelstork !

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

Successfully merging this pull request may close these issues.

5 participants