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

Migrate std::string APIs to const std::string & #627

Merged
merged 10 commits into from
Oct 14, 2015

Conversation

bp-kelley
Copy link
Contributor

Large ABI breaking migration of std::string API points to const std::string.

This removes a lot of string copies ( in the case of smiles/smarts it removes at least two ).

Special care should be taken to reviewing the smiles/smarts parsers, especially with FLEX/BISON on/off
-DRDK_USE_FLEXBISON=ON
there are a lot of moving parts.

@bp-kelley
Copy link
Contributor Author

At least it will take away one bullet point from Roger's slides for the UGM next year.

@bp-kelley
Copy link
Contributor Author

It looks like windows build needs to update the library path, where does the build recipe live?

LINK : fatal error LNK1104: cannot open file 'libboost_serialization-vc120-mt-1_56.lib'

@greglandrum
Copy link
Member

It's ok to ignore failures for the appveyor builds. Those are troubled anyway due to the runtime constraints on free appveyor builds

@@ -118,16 +118,13 @@ namespace RDKit{
}
}

void iterateCIPRanks(const ROMol &mol, DOUBLE_VECT &invars, UINT_VECT &ranks,bool seedWithInvars){
void iterateCIPRanks(const ROMol &mol, DOUBLE_VECT &invars,
Copy link
Member

Choose a reason for hiding this comment

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

Though the changes in this function seem to make sense after a brief review, I don't think they are connected to the const std::string fix that's the actual purpose of the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this, this is part of another optimization roll up I should have removed from this commit.

@bp-kelley
Copy link
Contributor Author

The original appveyor build passed if that helps:

https://ci.appveyor.com/project/bp-kelley/rdkit

greglandrum added a commit that referenced this pull request Oct 14, 2015
Migrate std::string APIs to const std::string &
@greglandrum greglandrum merged commit 59f6117 into rdkit:master Oct 14, 2015
@greglandrum
Copy link
Member

Yeah, sorry, this one got lost.
Thanks for making the changes.

@greglandrum greglandrum added this to the 2015_09_1 milestone Oct 14, 2015
mcs07 added a commit to mcs07/rdkit that referenced this pull request Jan 19, 2016
In rdkit#627, `readAmberTrajectory` in ConformerParser.h is updated to use `const std::string &`. This updates the corresponding method in ConformerParser.cpp to match. I needed to do this to compile RDKit with contrib.
@bp-kelley bp-kelley deleted the fix/const-string-ref branch March 19, 2016 13:51
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.

None yet

3 participants