refactor ProcessFactory.java to put all process creations in one function. #188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Prerequisite of #187 |
jonahgraham
reviewed
Nov 30, 2022
There was a problem hiding this comment.
This change looks reasonable. I find it a little hard to read due to all the inline comments, a builder pattern would certainly be preferred over the overly long number of incompatible arguments, but is not required. For clarity, I gave some examples in the comments of what I mean.
Once build and test approves it I will check closer to see if the translation of each method is correct.
Comment on lines
66
to
143
| Process p = doExec(true/*use_cmd*/, cmd, null/*cmdarray*/, null/*envp*/, null/*dir*/, false/*use_pty*/, | ||
| null/*pty*/, false/*has_gracefulExitTimeMs*/, 0/*gracefulExitTimeMs*/); | ||
| return p; |
There was a problem hiding this comment.
Suggested change
| Process p = doExec(true/*use_cmd*/, cmd, null/*cmdarray*/, null/*envp*/, null/*dir*/, false/*use_pty*/, | |
| null/*pty*/, false/*has_gracefulExitTimeMs*/, 0/*gracefulExitTimeMs*/); | |
| return p; | |
| return new Builder().setCmd(cmd).launch(); |
Comment on lines
147
to
216
| Process p = doExec(false/*use_cmd*/, null/*cmd*/, cmdarray, envp, dir, true/*use_pty*/, pty, | ||
| true/*has_gracefulExitTimeMs*/, gracefulExitTimeMs); | ||
| return p; |
There was a problem hiding this comment.
Suggested change
| Process p = doExec(false/*use_cmd*/, null/*cmd*/, cmdarray, envp, dir, true/*use_pty*/, pty, | |
| true/*has_gracefulExitTimeMs*/, gracefulExitTimeMs); | |
| return p; | |
| return new Builder().setCmdArray(cmd).setEnvp(envp).setCwd(dir).setPty(pty).launch(); |
jonahgraham
requested changes
Nov 30, 2022
There was a problem hiding this comment.
On further consideration, please refactor the code for readability. My reasoning is that all day I have had this in my review queue, and every time I come back to it I look at it for a few minutes and move on to something else due to the lack of code readability here.
Comment on lines
166
to
175
| if (use_cmd) { | ||
| if (cmd.isEmpty()) | ||
| throw new IllegalArgumentException("Empty command"); | ||
|
|
||
| StringTokenizer st = new StringTokenizer(cmd); | ||
| cmdarray = new String[st.countTokens()]; | ||
| for (int i = 0; st.hasMoreTokens(); i++) | ||
| cmdarray[i] = st.nextToken(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't make the non-deprecated methods go through the deprecated code handling. i.e. the exec(String[] cmdarray) variant should not have to deal with the use_cmd flag. My reasoning is the intention is to mark these deprecated methods for deletion one day, and I don't want me or a future programmer to have to detangle this then.
|
All exec() function redirected to a central place, the goal is to help
debugging.
To intercept all process creation, by setting one breakpoint.
…On Thu, Dec 1, 2022 at 5:37 AM Jonah Graham ***@***.***> wrote:
***@***.**** requested changes on this pull request.
On further consideration, please refactor the code for readability. My
reasoning is that all day I have had this in my review queue, and every
time I come back to it I look at it for a few minutes and move on to
something else due to the lack of code readability here.
------------------------------
In
core/org.eclipse.cdt.core.native/src/org/eclipse/cdt/utils/spawner/ProcessFactory.java
<#188 (comment)>:
> + if (use_cmd) {
+ if (cmd.isEmpty())
+ throw new IllegalArgumentException("Empty command");
+
+ StringTokenizer st = new StringTokenizer(cmd);
+ cmdarray = new String[st.countTokens()];
+ for (int i = 0; st.hasMoreTokens(); i++)
+ cmdarray[i] = st.nextToken();
+ }
+
Don't make the non-deprecated methods go through the deprecated code
handling. i.e. the exec(String[] cmdarray) variant should not have to
deal with the use_cmd flag. My reasoning is the intention is to mark these
deprecated methods for deletion one day, and I don't want me or a future
programmer to have to detangle this then.
—
Reply to this email directly, view it on GitHub
<#188 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5RCUAK4ASFMLWYG5H2SNTWK7CJJANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
But not all of them have to go through the string tokenizer
On Wed, Nov 30, 2022 at 4:46 PM 徐持恒 Xu Chiheng ***@***.***>
wrote:
All exec() function redirected to a central place, the goal is to help
debugging.
To intercept all process creation, by setting one breakpoint.
On Thu, Dec 1, 2022 at 5:37 AM Jonah Graham ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
>
> On further consideration, please refactor the code for readability. My
> reasoning is that all day I have had this in my review queue, and every
> time I come back to it I look at it for a few minutes and move on to
> something else due to the lack of code readability here.
> ------------------------------
>
> In
>
core/org.eclipse.cdt.core.native/src/org/eclipse/cdt/utils/spawner/ProcessFactory.java
> <#188 (comment)>:
>
> > + if (use_cmd) {
> + if (cmd.isEmpty())
> + throw new IllegalArgumentException("Empty command");
> +
> + StringTokenizer st = new StringTokenizer(cmd);
> + cmdarray = new String[st.countTokens()];
> + for (int i = 0; st.hasMoreTokens(); i++)
> + cmdarray[i] = st.nextToken();
> + }
> +
>
> Don't make the non-deprecated methods go through the deprecated code
> handling. i.e. the exec(String[] cmdarray) variant should not have to
> deal with the use_cmd flag. My reasoning is the intention is to mark
these
> deprecated methods for deletion one day, and I don't want me or a future
> programmer to have to detangle this then.
>
> —
> Reply to this email directly, view it on GitHub
> <
#188 (review)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AA5RCUAK4ASFMLWYG5H2SNTWK7CJJANCNFSM6AAAAAASPUORUM
>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#188 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFF2RFRVX5HQN2JRXEGVIDWK7DLJANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you commented.Message ID:
***@***.***>
--
~~~
Jonah Graham
Kichwa Coders
www.kichwacoders.com
|
|
only cmd -> cmdArray needs tokenizer.
On Thu, Dec 1, 2022 at 5:50 AM Jonah Graham ***@***.***>
wrote:
… But not all of them have to go through the string tokenizer
On Wed, Nov 30, 2022 at 4:46 PM 徐持恒 Xu Chiheng ***@***.***>
wrote:
> All exec() function redirected to a central place, the goal is to help
> debugging.
> To intercept all process creation, by setting one breakpoint.
>
> On Thu, Dec 1, 2022 at 5:37 AM Jonah Graham ***@***.***>
> wrote:
>
> > ***@***.**** requested changes on this pull request.
> >
> > On further consideration, please refactor the code for readability. My
> > reasoning is that all day I have had this in my review queue, and every
> > time I come back to it I look at it for a few minutes and move on to
> > something else due to the lack of code readability here.
> > ------------------------------
> >
> > In
> >
>
core/org.eclipse.cdt.core.native/src/org/eclipse/cdt/utils/spawner/ProcessFactory.java
> > <#188 (comment)>:
> >
> > > + if (use_cmd) {
> > + if (cmd.isEmpty())
> > + throw new IllegalArgumentException("Empty command");
> > +
> > + StringTokenizer st = new StringTokenizer(cmd);
> > + cmdarray = new String[st.countTokens()];
> > + for (int i = 0; st.hasMoreTokens(); i++)
> > + cmdarray[i] = st.nextToken();
> > + }
> > +
> >
> > Don't make the non-deprecated methods go through the deprecated code
> > handling. i.e. the exec(String[] cmdarray) variant should not have to
> > deal with the use_cmd flag. My reasoning is the intention is to mark
> these
> > deprecated methods for deletion one day, and I don't want me or a
future
> > programmer to have to detangle this then.
> >
> > —
> > Reply to this email directly, view it on GitHub
> > <
> #188 (review)
>,
> > or unsubscribe
> > <
>
https://github.com/notifications/unsubscribe-auth/AA5RCUAK4ASFMLWYG5H2SNTWK7CJJANCNFSM6AAAAAASPUORUM
> >
> > .
> > You are receiving this because you authored the thread.Message ID:
> > ***@***.***>
> >
>
> —
> Reply to this email directly, view it on GitHub
> <#188 (comment)>,
or
> unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AAFF2RFRVX5HQN2JRXEGVIDWK7DLJANCNFSM6AAAAAASPUORUM
>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
--
~~~
Jonah Graham
Kichwa Coders
www.kichwacoders.com
—
Reply to this email directly, view it on GitHub
<#188 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5RCUCV2GQJJTVRSVQAEWLWK7D35ANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Precisely. But all methods are calling the method with two extra arguments. |
|
That does not matter, that is just interface.
…On Thu, Dec 1, 2022 at 6:04 AM Jonah Graham ***@***.***> wrote:
Precisely. But all methods are calling the method with two extra arguments.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
Don't make the non-deprecated methods go through the deprecated code handling. i.e. the exec(String[] cmdarray) variant should not have to deal with the use_cmd flag. My reasoning is the intention is to mark these deprecated methods for deletion one day, and I don't want me or a future programmer to have to detangle this then. In the future I want to delete the deprecated code, I don't want to have to modify non-deprecated code when I do that. I won't accept this change as is and I would appreciate it if you fixed it. |
|
That can be done by leaving a comment in the code.
…On Thu, Dec 1, 2022 at 6:37 AM Jonah Graham ***@***.***> wrote:
Don't make the non-deprecated methods go through the deprecated code
handling. i.e. the exec(String[] cmdarray) variant should not have to deal
with the use_cmd flag. My reasoning is the intention is to mark these
deprecated methods for deletion one day, and I don't want me or a future
programmer to have to detangle this then.
In the future I want to delete the deprecated code, I don't want to have
to modify non-deprecated code when I do that. I won't accept this change as
is and I would appreciate it if you fixed it.
—
Reply to this email directly, view it on GitHub
<#188 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5RCUHNTA7T7QHY63RKCUDWK7JLRANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Thanks, but if you don't want to make this change, please close the PR. |
|
I'm confused, what change to make ? you can do whatever you want on that branch.
Adding comment ? or using build pattern ? or what ?
…On Thu, Dec 1, 2022 at 6:52 AM Jonah Graham ***@***.***> wrote:
Thanks, but if you don't want to make this change, please close the PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
The version of your code is hard to read, makes future maintenance harder. I don't mind how you fix it, I would use a builder pattern to make it easier. Having a method with 9+ command line arguments is excessive, as demonstrated by you needing to have comments on each line. Imagine in the future we need to add an additional argument to your new method, we then need to update many many places. I understand why you want to make this change, but the current version of the change is not good enough. If you disagree with my assessment, I will add an additional CDT committer to the review list for a second opinion.
I don't know what this means? Do you want me to change your change? |
|
The code is very simple, deleting the deprecated code just needs to
remove the dead branch in doExec().
below is your message:
Don't make the non-deprecated methods go through the deprecated code
handling. i.e. the exec(String[] cmdarray) variant should not have to
deal with the use_cmd flag. My reasoning is the intention is to mark
these deprecated methods for deletion one day, and I don't want me or
a future programmer to have to detangle this then.
In the future I want to delete the deprecated code, I don't want to
have to modify non-deprecated code when I do that. I won't accept this
change as is and I would appreciate it if you fixed it.
…On Thu, Dec 1, 2022 at 6:58 AM 徐持恒 Xu Chiheng ***@***.***> wrote:
I'm confused, what change to make ? you can do whatever you want on that branch.
Adding comment ? or using build pattern ? or what ?
On Thu, Dec 1, 2022 at 6:52 AM Jonah Graham ***@***.***> wrote:
>
> Thanks, but if you don't want to make this change, please close the PR.
>
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
I can work on it when I have time, I'm now working on something else.
In summary, the code is extremely simple, changing it is also very simple.
…On Thu, Dec 1, 2022 at 7:03 AM Jonah Graham ***@***.***> wrote:
The version of your code is hard to read, makes future maintenance harder.
I don't mind how you fix it, I would use a builder pattern to make it
easier. Having a method with 9+ command line arguments is excessive, as
demonstrated by you needing to have comments on each line. Imagine in the
future we need to add an additional argument to your new method, we then
need to update many many places. I understand why you want to make this
change, but the current version of the change is not good enough.
If you disagree with my assessment, I will add an additional CDT committer
to the review list for a second opinion.
you can do whatever you want on that branch.
I don't know what this means? Do you want me to change your change?
—
Reply to this email directly, view it on GitHub
<#188 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5RCUAUICRP7HP4NGOW6ZLWK7MLZANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Excellent. I am glad the change is simple for you to make. I look forward to a change that is readable, maintainable and adds the functionality you want. |
jonahgraham
approved these changes
Dec 1, 2022
…tion. Prerequisite of #187
|
Because there were two unrelated topics in this PR I wasn't able to merge it directly from the web interface, so I squashed and forced pushed the main change, and put the other change in #190 |
|
I thought it was because of the 2 strings with NO-NLS comments
…On Sat, Dec 3, 2022 at 11:45 PM Jonah Graham ***@***.***> wrote:
Because there were two unrelated topics in this PR I wasn't able to merge
it directly from the web interface, so I squashed and forced pushed the
main change, and put the other change in #190
<#190>
—
Reply to this email directly, view it on GitHub
<#188 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5RCUB2D652VSZHVN7OW7LWLNTLBANCNFSM6AAAAAASPUORUM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Our CI isn't setup to catch new warnings, so I didn't notice that. It would be great if you fixed it. |
jonahgraham
approved these changes
Dec 3, 2022
|
Thanks for the improvement. I hope this refactor makes it easier for you to debug and develop the solution for #187 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.