Passing variables by references to the map about cparse

[VinGarcia]

Hello Jean, thanks for the suggestion.

Can you exemplify your use-case? I think this can be a worthy contribution, however, we need to consider the pros and cons.

For example, if we add this new feature we'll add complexity to the framework which might complicate the documentation for new users.

On the other hand, if there is a use-case that we can estimate to be a common need and that will make the library more useful for a good number of our users, this extra complexity might be worth it.

Also, please submit a Pull Request, it will be nice to be able to see your code and we can continue the discussion there.

In either case, thank you for your contribution and interest.

[jpfeger]

Hi Vinicius, A bit of backgroung before answering your points. I am a software engineer working for CAE, a Montreal based, large company in the simulation and training fields (https://www.cae.com/). I am currently evaluating the Cparse library under Visual Studio 2017 for a potential use in our frameworks. To understand in a glance the additions from a userès perspective, you may take a look at the few Google tests that I wrote (in attachement). I haven't yet put the necessary MIT licence statements though :-) Surely I would like to propose the contributions through a pull request. Before that to happen: 1 - I need to compile the updated library under your environment .. which is? 2 - I need to expand the changes to other plain types beside bool, double, and integer. I know that String is missing. I am not sure about the rest. Which other ones would you like me to add? 3 - I'd like to visit what really happens when a calculation is done on dangling references. I was expecting crashes... which did not occurred. I feel a bit uneasy with that There is fourth point I'd like to discuss with you also. I tested the calculation's performances... and they are disappointing, which came as a surprise. I'd definitively like to tackle the issue... Was it ever reported to you? has someone already checked that? Thanks for your feedback. Jean-Pierre. Le dim. 13 janv. 2019, à 12 h 38, Vinícius Garcia <[email protected]> a écrit :

…

Hello Jean, thanks for the suggestion. Can you exemplify your use-case? I think this can be a worthy contribution, however, we need to consider the pros and cons. For example, if we add this new feature we'll add complexity to the framework which might complicate the documentation for new users. On the other hand, if there is a use-case that we can estimate to be a common need and that will make the library more useful for a good number of our users, this extra complexity might be worth it. Also, please submit a Pull Request, it will be nice to be able to see your code and we can continue the discussion there. In either case, thank you for your contribution and interest. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsWsASUC-Rc2gtw16IdxY3y2kagAcks5vC28ygaJpZM4Z8eBE> .

[VinGarcia]

Hello @jpfeger, sorry for taking so long to answer,

1. The TravisCI environment compiles it under GCC 4.8 on an Ubuntu VM.
2. Since the only types that are passed by value are strings, integers, booleans and doubles if you implement this change for strings it would be perfect.
3. I also considered that. If it did not crash it might be one of those cases where it is using a pointer to something that was already deleted but is still there; this might cause hard-to-debug problems if the user uses it without understanding how references work.

About the performance, I haven't tested it but I kind of expected it not to be very fast. There are several optimizations I want to do that would improve it in a little bit but the code was made to be flexible and very easy to use, so it does have some problems on this area.

For example, all the operations are saved on maps that associate strings to function pointers, each time an operation has to be performed it has to access the map, retrieve the function sometimes wrap the parameters on a packToken and then run it.

The bright side is that if you want to specialize the framework for your own programming language you should be able to remove most of the abstraction layers and it would become much faster for sure.

So, long story short, it was meant to be flexible and modular, not fast. I would appreciate help if you have the time, I think the main point where we could optimize it is on how the tokens are stored on the memory, probably using a union type would be more efficient than the way it is done today with polymorphism and inheritance.

[jpfeger]

Sure, I'ill add the missing type, the string. Since the developments are charged on my company's R&D I have a limited amount of time to spend. So I tend to focus on the essential (the extensions) and therefore I am not sure I'll have the time to install, configure an Ubuntu environment. For sure I will share whatever I have added to the library in a few days or weeks, The extended code under Visual Studio being close to the original one, in the worst case (no GCC), you would have no issue merging the changes. As for the performances which is a show stopper for me right now , I plan to instrument the code, spot the trouble makers and fix them. Before that I need to present my intents to my bosses for approval. Surely I'll keep you posted on my progress. Thanks Vinicius. Jean-Pierre. Le mar. 22 janv. 2019, à 20 h 01, Vinícius Garcia <[email protected]> a écrit :

…

Hello @jpfeger <https://github.com/jpfeger>, sorry for taking so long to answer, 1. The TravisCI environment compiles it under GCC 4.8 on an Ubuntu VM. 2. Since the only types that are passed by value are strings, integers, booleans and doubles if you implement this change for strings it would be perfect. 3. I also considered that. If it did not crash it might be one of those cases where it is using a pointer to something that was already deleted but is still there; this might cause hard-to-debug problems if the user uses it without understanding how references work. About the performance, I haven't tested it but I kind of expected it not to be very fast. There are several optimizations I want to do that would improve it in a little bit but the code was made to be flexible and very easy to use, so it does have some problems on this area. For example, all the operations are saved on maps that associate strings to function pointers, each time an operation has to be performed it has to access the map, retrieve the function sometimes wrap the parameters on a packToken and then run it. The bright side is that if you want to specialize the framework for your own programming language you should be able to remove most of the abstraction layers and it would become much faster for sure. So, long story short, it was meant to be flexible and modular, not fast. I would appreciate help if you have the time, I think the main point where we could optimize it is on how the tokens are stored on the memory, probably using a union type would be more efficient than the way it is done today with polymorphism and inheritance. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsYYRwWOW5sLBQB5IoEaS7-OvA3NOks5vF7RTgaJpZM4Z8eBE> .

[jpfeger]

Hi Vinicius, a few updates: 1 - did not get yet my boss's approval for continuing as we are waiting to clear a dll dependency first, In fact, it won't change anything as I'll proceed anyway on my personal time :-) 2 - installing a VM seems too time consuming. I plan instead to merge myself the changes from my VisualStudio repo to specific branches under https://github.com/cparse/cparse . I'll let you then make sure that it compiles before merging to master. Should not be troublesome 3 - I plan to offer the changes with reference_wrapper for all the plain types sometime next week. 4 - for the performance issues, I will be using the Visual Studio profiler and hopefully will have that fixed in February 5 - I checked the issue with reference_wrapper and dangling references. I was expecting a crash when assigning a descoped variables but none occurred. It turns out that the wrapper extends the lifetime of the variable (see https://stackoverflow.com/questions/23973439/why-does-stdreference-wrapperconst-t-not-accept-a-temporary ) 6 - I guess I would need to be registered to github cparse to be able to push my branch. Not sure if it needs to use SSH key. Let me know how to proceed Jean-Pierre, Le mar. 22 janv. 2019, à 23 h 09, Jean-Pierre Feger <[email protected]> a écrit :

…

Sure, I'ill add the missing type, the string. Since the developments are charged on my company's R&D I have a limited amount of time to spend. So I tend to focus on the essential (the extensions) and therefore I am not sure I'll have the time to install, configure an Ubuntu environment. For sure I will share whatever I have added to the library in a few days or weeks, The extended code under Visual Studio being close to the original one, in the worst case (no GCC), you would have no issue merging the changes. As for the performances which is a show stopper for me right now , I plan to instrument the code, spot the trouble makers and fix them. Before that I need to present my intents to my bosses for approval. Surely I'll keep you posted on my progress. Thanks Vinicius. Jean-Pierre. Le mar. 22 janv. 2019, à 20 h 01, Vinícius Garcia < ***@***.***> a écrit : > Hello @jpfeger <https://github.com/jpfeger>, sorry for taking so long to > answer, > > 1. The TravisCI environment compiles it under GCC 4.8 on an Ubuntu VM. > 2. Since the only types that are passed by value are strings, > integers, booleans and doubles if you implement this change for strings it > would be perfect. > 3. I also considered that. If it did not crash it might be one of > those cases where it is using a pointer to something that was already > deleted but is still there; this might cause hard-to-debug problems if the > user uses it without understanding how references work. > > About the performance, I haven't tested it but I kind of expected it not > to be very fast. There are several optimizations I want to do that would > improve it in a little bit but the code was made to be flexible and very > easy to use, so it does have some problems on this area. > > For example, all the operations are saved on maps that associate strings > to function pointers, each time an operation has to be performed it has to > access the map, retrieve the function sometimes wrap the parameters on a > packToken and then run it. > > The bright side is that if you want to specialize the framework for your > own programming language you should be able to remove most of the > abstraction layers and it would become much faster for sure. > > So, long story short, it was meant to be flexible and modular, not fast. > I would appreciate help if you have the time, I think the main point where > we could optimize it is on how the tokens are stored on the memory, > probably using a union type would be more efficient than the way it is > done today with polymorphism and inheritance. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#65 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AfZSsYYRwWOW5sLBQB5IoEaS7-OvA3NOks5vF7RTgaJpZM4Z8eBE> > . >

[VinGarcia]

Hello @jpfeger, sorry for the delay,

About 6: For you to make a Pull Request (PR for short) on CParse you just need to fork the repository on your own account. Then you should be able to submit a PR, here's the documentation:

• https://help.github.com/en/articles/creating-a-pull-request-from-a-fork

About 2: Don't worry if the feature is good I will fix any problems on the Linux compilation, just make the PR and we talk about it.

About 4: If you are able to improve the performance it would be great, but it would be better to create a separated branch and a separated PR so if one of them is rejected the other can still be merged.

Again, I am sorry for disappearing, I am not having much free time =/

[jpfeger]

No problem for the delay. I was quite busy myself these days. I attempted a few time to push a branch but was not allowed to proceed (error: 403). Not sure if it relates to my credentials ... could you check whether my two accounts on Git Hub ([email protected] + jean-pierre.feger@cae,com) allow me to push something to the cparse repo? Also, I am not sure about the exact way to push .. do we need an SSH key? I am quite familiar with sourcetree but could you use git command lines either. I guess a short description with a few git commands would be quite helpful. Thanks. JP Le jeu. 21 févr. 2019, à 18 h 44, Vinícius Garcia <[email protected]> a écrit :

…

Hello @jpfeger <https://github.com/jpfeger>, sorry for the delay, About 6: For you to make a Pull Request (PR for short) on CParse you just need to fork the repository on your own account. Then you should be able to submit a PR, here's the documentation: - https://help.github.com/en/articles/creating-a-pull-request-from-a-fork About 2: Don't worry if the feature is good I will fix any problems on the Linux compilation, just make the PR and we talk about it. About 4: If you are able to improve the performance it would be great, but it would be better to create a separated branch and a separated PR so if one of them is rejected the other can still be merged. Again, I am sorry for disappearing, I am not having much free time =/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsVzualBFg_12dthyl6XQp94j6Urcks5vPy9rgaJpZM4Z8eBE> .

[VinGarcia]

You cannot push into someone else's repo, but you always push on your own repo and ask me to pull your changes into my repo, that's the concept of the Pull Request.

The SSH key unrelated to this process of creating a PR, it is only used to allow you to make pulls and pushes without ever needing to type your password, it is quite handy, but not a necessity for anything.

So I don't know if it is possible to make a PR via command line, but I know you will have to push your changes on one of your own branches, the usual workflow goes like this:

# Note that I am cloning your repo, not mine:
git clone https://github.com/jpfeger/cparse.git myCparseClone
cd myCparseClone/

# Create a new branch with a descriptive name:
git checkout -b optimize-cparse-performance

# ... make some changes on the code ...

# Commit your changes (preferably with multiple small commits instead of a single big one)
git add some-files
git commit -m 'optimized function foo()'
git push origin optimize-cparse-performance

# Then go to github.com/jpfeger/cparse and click on "new pull request"
# From there on it should be straight forward

[jpfeger]

I am pretty familiar with Git pull, commit, push as they are part of my daily activity. Still I cannot push my local branch to the remote Cparse repo likely for some right access. Images being worth thousands of words, see my steps: *The push operation itself: * [image: image.png] *then, the login: * [image: image.png] *and finally the failure:* [image: image.png] Note that entering ther correct user with a wrong password does not lead to the same sequence as the login is still required for the subsequent pushes. This leads me to think that my credentials to git hub are fine, but that I dont have the rights to push my local branch in Cparse.. JP Le ven. 1 mars 2019, à 08 h 15, Vinícius Garcia <[email protected]> a écrit :

…

You cannot push into someone else's repo, but you always push on your own repo, and ask me to pull your changes into my repo, that's the concept of the Pull Request. The SSH key unrelated to this process of creating a PR, it is only used to allow you to make pull and pushes without ever needing to type your password, it is quite handy, but not a necessity for anything. So I don't know if it is possible to make a PR via command line, but I know you will to push your changes on one of your own branches, the usual workflow goes like this: # Note that I am cloning your repo, not mine: git clone https://github.com/jpfeger/cparse.git myCparseClonecd myCparseClone/ # Create a new branch with a descriptive name: git checkout -b optimize-cparse-performance # ... make some changes on the code ... # Commit your changes (preferrably with multiple small commits instead of a single big one) git add some-files git commit -m 'optimized function foo()' git push origin optimize-cparse-performance # Then go to github.com/jpfeger/cparse and click on "new pull request"# From there on it should be straight forward — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsTRsMkC5bljikTMR8KeAQZCwq3ycks5vSSf9gaJpZM4Z8eBE> .

[VinGarcia]

I actually did not get the images but I am pretty sure I know what is wrong.

The first thing is that it is not normal to push to someone else's repo. I could give you permission but it would also give you permission to do a lot of stuff on the repo that might not be desirable. So this isn't the usual way.

What far more common is the concept of a Pull Request or PR, this is a more elegant way to offer code changes on a open-source project:

You make a fork, like this one you already have:

• https://github.com/jpfeger/cparse.git

Make your changes and push them to your own fork, not the official CParse repo.

And then you use a special feature from Github called Pull Request or PR where I can see your code, suggest changes, run tests and etc before I allow any changes to happen on the official code.

Then if I am satisfied I click on a special button to accept your changes and merge them into my master branch.

You can't do a PR on the command line as far as I know since it is a Github feature, not git.
Also, the Github interface is so easy that it should be easy to use.

I found a tutorial online with pictures showing how to make a Pull Request, you may skip to section 8 since there is where he explains the PR part:

• https://opensource.guide/how-to-contribute/

[VinGarcia]

@jpfeger here's an example of a PR I just created to submit some changes on an open-source project of a friend of mine:

• arthurbailao/aws-kcl#18

[jpfeger]

... right, there was some misundertandings on my part regarding how git hub works for collaborative developments. I forked the Cparse repo jn my own space, pushed my changes to it, and created a pull request without any reviewer for now. I sent you any invitation to fill the gap. JP Le mer. 6 mars 2019, à 11 h 02, Vinícius Garcia <[email protected]> a écrit :

…

@jpfeger <https://github.com/jpfeger> here's an example of a PR I just created to submit some changes on an open-source project of a friend of mine: - arthurbailao/aws-kcl#18 <arthurbailao/aws-kcl#18> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsZnwJq7CxB60ubVD_oR9iU_RTk0Wks5vT-aEgaJpZM4Z8eBE> .

[VinGarcia]

Yeah, no problem, I will take a look.

[jpfeger]

Great ;-) As soon as you accept my invitation I'll be able to send my pull request. [image: image.png] Le lun. 11 mars 2019, à 20 h 09, Vinícius Garcia <[email protected]> a écrit :

…

Yeah, no problem, I will take a look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsbTq1fZMgvokcMFWu1a4Ggi-72Amks5vVvA2gaJpZM4Z8eBE> .

[jpfeger]

Hi Vinicius, I am currently completely stalled: unless I missed something here, there is no way for you to review my changes through a pul request without you first accepting my invitation as a contributor to my forked repo ( https://github.com/jpfeger/cparse/invitations ) JP Le lun. 11 mars 2019, à 20 h 09, Vinícius Garcia <[email protected]> a écrit :

…

Yeah, no problem, I will take a look. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AfZSsbTq1fZMgvokcMFWu1a4Ggi-72Amks5vVvA2gaJpZM4Z8eBE> .