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

Refactoring .hpp -> .h, .cpp and remove class templates #45

Merged
merged 6 commits into from
Sep 28, 2023

Conversation

mrp089
Copy link
Member

@mrp089 mrp089 commented Sep 27, 2023

This is equivalent to #41 but leaves the file history intact. Closes #36.

Do not squash and merge to leave history intact.

@mrp089 mrp089 mentioned this pull request Sep 27, 2023
@mrp089
Copy link
Member Author

mrp089 commented Sep 28, 2023

Steps to create new files while preserving the history:

  1. Create .cpp files from .hpp
git mv FILE.hpp FILE.cpp
git commit
  1. Reset .hpp files
SAVED=`git rev-parse HEAD`
git reset --hard HEAD^
  1. Create .h files from .hpp
git mv FILE.hpp FILE.h
git commit
  1. Merge commits to have both .cpp and .h files
git merge $SAVED
git commit -a

@mrp089
Copy link
Member Author

mrp089 commented Sep 28, 2023

I manually recreated the file structure in @ktbolt's branch using git mv.

Since we can't do a squash and merge in this pull request, I already squashed @ktbolt's 21 commits into one using an interactive rebase (marking all but the first commit as squash):

git rebase -i HEAD~25

Finally, I applied @ktbolt's changes by creating a diff to the rebased branch with git diff and applied it as a patch with git apply to only change file contents but not the files themselves. I then commited that with @ktbolt as an author - the perfect crime!

@mrp089
Copy link
Member Author

mrp089 commented Sep 28, 2023

Note that there are still many files shown as newly created. However, this is just a GitHub problem (I assume it can't show two files as moved from the same original one).

If you do a git blame on one of the "newly created" files (e.g. BloodVessel), you get the full history.

@mrp089
Copy link
Member Author

mrp089 commented Sep 28, 2023

@ktbolt, @menon-karthik I'll fix the Doxygen documentation. It doesn't find the files anymore and many comments need to be updated.

@mrp089
Copy link
Member Author

mrp089 commented Sep 28, 2023

Done! There are a few more Doxygen warnings, see #46, but nothing that should prevent this from being merged.

Copy link
Member

@menon-karthik menon-karthik left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me! I just have a few comments.

Thanks @ktbolt and @mrp089 for doing this!


# Need to add the -fPIC flag to build the interface shared library.
if(CMAKE_CXX_COMPILER_ID MATCHES "GNU")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")
Copy link
Member

Choose a reason for hiding this comment

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

@ktbolt Just wondering why this wasn't required previously?

Copy link
Contributor

Choose a reason for hiding this comment

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

@menon-karthik The current version creates object files that are linked into the interface shared libraries so they need to have relative addresses.


#include "Node.h"

namespace zd_model {
Copy link
Member

@menon-karthik menon-karthik Sep 28, 2023

Choose a reason for hiding this comment

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

@ktbolt Do we really need to call this zd_model instead of just model?

Copy link
Contributor

Choose a reason for hiding this comment

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

@menon-karthik Oops, meant to rename that, was testing for name collision problems. I plan on removing namespaces in a later Issue.

Copy link
Member

Choose a reason for hiding this comment

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

Git thinks many of these files are copied from src/io/namespace

Copy link
Member

Choose a reason for hiding this comment

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

Just pointing this out, not saying it's something we should dig into.

Copy link
Member Author

@mrp089 mrp089 Sep 28, 2023

Choose a reason for hiding this comment

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

Yeah, that's weird. But I think it's just GitHub's diff display. In the history, it shows up correctly.

@menon-karthik menon-karthik changed the title Refactoring .hpp -> .h, .cpp Refactoring .hpp -> .h, .cpp and remove class templates Sep 28, 2023
@ktbolt
Copy link
Contributor

ktbolt commented Sep 28, 2023

@mrp089 Thank you for getting all of this to work, my partner in crime!

Copy link
Contributor

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks a bunch @mrp089 and @menon-karthik for pushing this messy PR through.

@ktbolt ktbolt merged commit df6a603 into SimVascular:master Sep 28, 2023
6 checks passed
@mrp089 mrp089 deleted the refactor_patch branch September 28, 2023 18:42
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.

Remove class templates
3 participants