Skip to content

bugfix: parameter forwarding in operator= on rvalue and don't implement… - #34

Open
roman-stratoscale wants to merge 3 commits into
shlomimatichin:masterfrom
Stratoscale:master
Open

bugfix: parameter forwarding in operator= on rvalue and don't implement…#34
roman-stratoscale wants to merge 3 commits into
shlomimatichin:masterfrom
Stratoscale:master

Conversation

@roman-stratoscale

Copy link
Copy Markdown

… move constructor

Signed-off-by: Roman roman@stratoscale.com

… move constructor

Signed-off-by: Roman <roman@stratoscale.com>
@shlomimatichin

Copy link
Copy Markdown
Owner

Hi Roman, Welcome to the team! I have some feedback for you, please don't let this demotivate you, i would very much like to encourage participation and contribution!

  1. I would like to see a test case for the issue you raised. Me and ronnie (especially ronnie) write a test case for any modification to the parser or generator. The simplest way would be for ronnie to show you have to write such an example "project", which also acts as a tests suite. This is particularly important for this case, as i'm really not sure i understand what you are trying to solve - and test cases are a way to communicate this information.
  2. There are two modifications here, joined into a single commit: Skip implementing move constructor, and fix an issue you had with operator=. In the future, please create two commits (same pull request is fine, but two commits helps us work together, remotely). (BTW, small typo in commit message).
  3. The first part, not implementing a move constructor, is inconsistent with voodoo's design so far. So far, every class, even if it has a copy constructor, didn't have one, or even if it had a deleted copy constructor, would receive a copy constructor implementation. This makes working with mocks in the test suite "lightweighted" (e.g., before C++11, you could create a vector of mocks, even if the real class had a private copy constructor). I think this paradigm should be preserved. Also, the copy constructor adds "Copy of" to the voodooName, and i think the move constructor should destroy the moved out object voodooName, to keep this behaviour more intuitive to test writers.
  4. The second fix, "parameter forwarding in operator =" has few issues with it, i don't think this implementation can be merged: first, you triggered this behaviour for any method that has one && reference to the same class, not just operator =. this is why the sample would have helped me, i'm not sure if this was intentional. if so, the commit message is confusing, if not, then it's a bug. Now your solution checks to trigger this by the function decomposition, while i'm pretty sure it needs to be something regarding the parameter itself (lets say you have a method that receives two && arguments. whats the reasoning for them to be treated otherwise than a method with one?). I think the right location to implement it is inside the function decomposition class, btw. So let's recap: i'm not sure what the solution should be, i would like to work with an example (==test case). Then i can offer a much more specific solution, but this one tips off some alarms in my head...
  5. Finally, if you care for such things, create your pullrequest from your private github account, and start accumulating patches to open-source projects. This is a smart move, career wise.

Ronnie Lazar and others added 2 commits December 22, 2015 17:18
discard special words (static, inline...) when detrmening return type

Signed-off-by: Roman <roman@stratoscale.com>
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