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

Provide a macro to override the use of std::cerr #508

Merged
merged 4 commits into from
Oct 1, 2023

Conversation

jlmelville
Copy link
Contributor

#484 introduced a call to std::cerr when M is excessively large. This makes life hard for the R bindings because CRAN checks for and does not like to find anything writing to stdout/stderr (these streams should be sent to the console instead). Fortunately, the Rcpp library provides alternative streams (e.g. Rcpp::Rcout and Rcpp::Rcerr).

I propose using a macro to define the stream. By default this will be std::cerr, so users of this library need do nothing and will notice no changes. In RcppHNSW, I will create a header file which will declare the override, e.g.:

#include <Rcpp.h>

#define HNSWLIB_ERR_OVERRIDE Rcpp::Rcerr

#include "hnswlib.h"

A similar arrangement works for Annoy and RcppAnnoy.

@yurymalkov
Copy link
Member

Hi @jlmelville,

Thank you for the PR and sorry for the late response!

Looks good to me, but can you please add a link (it can be the link to this PR)?

Thanks!

@jlmelville
Copy link
Contributor Author

jlmelville commented Sep 29, 2023

@yurymalkov do you mean add a link to this PR in the comment to explain why the macro has been created? No problem to do so, just want to make sure I understand the request. (edit: I have updated the PR to include a link to this PR in the comment anyway as it seems like a good idea).

@yurymalkov yurymalkov merged commit c4418ea into nmslib:develop Oct 1, 2023
15 checks passed
@yurymalkov
Copy link
Member

Thank you!

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.

2 participants