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

Added move constructor for MIPSImage #13

Merged
merged 10 commits into from
Apr 27, 2023
Merged

Added move constructor for MIPSImage #13

merged 10 commits into from
Apr 27, 2023

Conversation

Nydauron
Copy link
Contributor

This PR adds a move constructor to MIPSImage.

There is some debate about whether to add a copy constructor (which would come in handy if we don't want to prepare the original assembly file, can technically (although seems rather inefficiently) allow the ability to rollback the state of the MIPS program to an earlier point in time).

Adding a copy constructor would require the use of malloc() to make sure that it is a deep copy rather than a shallow copy. It is sort of unclear as to what needs to be copied over as there are a lot of source pointers (pointers that were created by malloc()) and referential pointers (pointers that were shallow copied).

Copy link
Contributor

@philvukovic philvukovic left a comment

Choose a reason for hiding this comment

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

changes look good. but from my understanding, a move assignment operator should also be implemented when we add a move constructor, so I would suggest we add that

spim/CPU/image.h Show resolved Hide resolved
spim/CPU/image_print_stream.h Show resolved Hide resolved
@philvukovic
Copy link
Contributor

philvukovic commented Apr 26, 2023

There is some debate about whether to add a copy constructor (which would come in handy if we don't want to prepare the original assembly file, can technically (although seems rather inefficiently) allow the ability to rollback the state of the MIPS program to an earlier point in time).

This could be added in the future if it's desired, but it doesn't seem necessary right now, and there are some potential complications in performing a full deep copy of the image. Move semantics fit the average use case better

@Nydauron
Copy link
Contributor Author

This could be added in the future if it's desired, but it doesn't seem necessary right now, and there are some potential complications in performing a full deep copy of the image. Move semantics fit the average use case better

Sg. Created issue #14

@Nydauron
Copy link
Contributor Author

Added assignment operators. Used the following code to test it:

diff --git a/spim/worker.cpp b/spim/worker.cpp
index 89fe55a..ea59df7 100644
--- a/spim/worker.cpp
+++ b/spim/worker.cpp
@@ -82,8 +82,10 @@ void reset(unsigned int max_contexts, std::set<unsigned int> &active_ctxs) {
         char file_name[64];
         sprintf(file_name, "./input_%d.s", i);
         if (read_assembly_file(new_image, file_name)) { // check if the file exists
-            new_image.reg_image().PC = starting_address(new_image);
-            ctxs.emplace(i, std::move(new_image));
+            MIPSImage img(i);
+            img = std::move(new_image);
+            img.reg_image().PC = starting_address(img);
+            ctxs.emplace(i, std::move(img));
         }
         yylex_destroy();
     }

Albeit it is rather inefficient, hence why its only being used for testing and not for production

For compiling for native, I also added the following to main() (mainly for the purposes of Valgrind):

diff --git a/main.cpp b/main.cpp
index d163359..417e703 100644
--- a/main.cpp
+++ b/main.cpp
@@ -1,2 +1,18 @@
 // Need to provide main() function for JsSpim to work
-int main() {}
+#include "spim/worker.h"
+#include <set>
+int main() {
+    std::set<unsigned int> active_ctxs({0, 1});
+
+    reset(2, active_ctxs);
+    reset(2, active_ctxs);
+    active_ctxs.clear();
+    active_ctxs.insert(1);
+    reset(2, active_ctxs);
+    active_ctxs.clear();
+    active_ctxs.insert(0);
+    reset(2, active_ctxs);
+    shutdown();
+
+    exit(0);
+}

@Nydauron Nydauron requested a review from philvukovic April 26, 2023 22:21
Copy link
Contributor

@philvukovic philvukovic left a comment

Choose a reason for hiding this comment

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

wanted some clarification on the change of sink from & to *. maybe this is fine for our use case, but it should be well-documented how the ostream is managed as it is a bit counter-intuitive

spim/CPU/image_print_stream.h Show resolved Hide resolved
@Nydauron Nydauron requested a review from philvukovic April 26, 2023 23:33
spim/CPU/image_print_stream.h Show resolved Hide resolved
@Nydauron Nydauron merged commit 6d7e43c into master Apr 27, 2023
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