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

failed to initialize with substr #46

Merged
merged 3 commits into from
Feb 25, 2022

Conversation

biwiki
Copy link
Collaborator

@biwiki biwiki commented Feb 20, 2022

Couple of bug fixes

@biwiki biwiki added bug Something isn't working c++17 migration labels Feb 20, 2022
@biwiki biwiki requested a review from aregtech February 20, 2022 11:26
@biwiki biwiki self-assigned this Feb 20, 2022
Copy link
Owner

@aregtech aregtech left a comment

Choose a reason for hiding this comment

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

@biwiki , please have a look at comment.

@@ -44,14 +44,17 @@ void Process::_initPaths( const char * fullPath )
if ( pos != std::string::npos )
{
mProcessPath = mProcessFullPath.substr(0, pos);
mProcessName = mProcessFullPath.substr(pos + 1);
// looks weird, but substr does not work without <str>
std::string str = mProcessFullPath.substr(pos + 1);
Copy link
Owner

Choose a reason for hiding this comment

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

@biwiki, if we change the type of mProcessName and other members from String to std::string, there will be no need for intermediate variable, which has impact on speed and memory (defragmentation). Can you please change the type of object members?

mProcessPath = std::filesystem::absolute(fullProcessPath).parent_path().c_str();
mProcessName = std::filesystem::absolute(fullProcessPath).filename().c_str();
mAppName = std::filesystem::absolute(fullProcessPath).stem().c_str();
mProcessExt = std::filesystem::absolute(fullProcessPath).extension().c_str();
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can optimize here, because mProcessFullPath = std::filesystem::absolute(fullProcessPath) and there is no need to re-calculate the full path.
@biwiki, can you please fix this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I still have something to do with this method, build is failing for Windows and I will fix it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I think this class should hold an instance of std::filesystem::path for the full process path. Other information can be extracted at runtime via C++17 filesystem. Different string members in the current class declaration is kind of duplicated info.

@aregtech
Copy link
Owner

The MS Build failed.

@aregtech
Copy link
Owner

I'll make some changes later. One test build fails because of MFC dependencies.

@aregtech aregtech merged commit de1f996 into 20220206-stl17-migration Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants