Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 18:08:05 GMT, Kevin Walls wrote: >> But on the other hand we have `javax.script.Invocable`. :-) >> >> Codespell suggested this change, and I based my decision to keep it based on >> [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not >> even listing "invokable" as an alternate spelling, and that "invocable" has >> about 3x the number of Google hits than "invokable". >> >> But sure, there is perhaps reason to consider "invokable" an acceptable >> alternative and keep it. I guess it depends on if you consider the word to >> be based on "invoke" with a suffix, or a latinized form, like "invocation". >> >> I'll wait a while for others to chime in, otherwise I'll revert the >> "invokable" changes. > > Sure, I just thought there was enough evidence that invokable is not > definitely wrong, even if invocable is more correct and popular, so it's not > obvious it should be changed. Don't lose sleep over it. 8-) > > More importantly, on the tests -- I saw the changes in exception messages, > searched for the wrong text in the test directories, and didn't find any > matches that looked like checks. Invocable, Invokable and Invokeable are all used in practice. We have a mix of invocable and invokable throughout our codebase, with more of the former than the latter - and in prose "invocable" is probably best. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. Hi Magnus, Great job, thank you for doing this! It looks good to me. These findings are pretty interesting. Some are funny and some seems to be typical. Good examples of typical typos are: - double letter instead of single - single or even triple letter instead of double Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 16:30:42 GMT, Daniel Fuchs wrote: > > @dfuch I have only updated files in `src`, so if the incorrect spelling is > > tested, that test will now fail. I'm unfortunately not well versed in what > > tests cover serviceability code. Can you suggest a suitable set of tests to > > run? > > Folks from serviceability-dev will be more able to answer that - but I would > suggest running tier2-tier3 as a precaution. I sent Magnus a PM with info on all the svc tests that can be run. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 17:22:04 GMT, Magnus Ihse Bursie wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: >> >>> 36: jboolean pending; /* Is an invoke requested? */ >>> 37: jboolean started; /* Is an invoke happening? */ >>> 38: jboolean available;/* Is the thread in an invocable state? */ >> >> invocable could perhaps stay as invokable ? >> Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which >> we clearly don't want to change, and I see Internet dictionary evidence of >> invokable being acceptable. > > But on the other hand we have `javax.script.Invocable`. :-) > > Codespell suggested this change, and I based my decision to keep it based on > [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not > even listing "invokable" as an alternate spelling, and that "invocable" has > about 3x the number of Google hits than "invokable". > > But sure, there is perhaps reason to consider "invokable" an acceptable > alternative and keep it. I guess it depends on if you consider the word to be > based on "invoke" with a suffix, or a latinized form, like "invocation". > > I'll wait a while for others to chime in, otherwise I'll revert the > "invokable" changes. Sure, I just thought there was enough evidence that invokable is not definitely wrong, even if invocable is more correct and popular, so it's not obvious it should be changed. Don't lose sleep over it. 8-) More importantly, on the tests -- I see the changes in exception messages searched for the wrong text in the test directories, and didn't find any matches that looked like checks. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 16:17:20 GMT, Kevin Walls wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: > >> 36: jboolean pending; /* Is an invoke requested? */ >> 37: jboolean started; /* Is an invoke happening? */ >> 38: jboolean available;/* Is the thread in an invocable state? */ > > invocable could perhaps stay as invokable ? > Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which > we clearly don't want to change, and I see Internet dictionary evidence of > invokable being acceptable. But on the other hand we have `javax.script.Invocable`. :-) Codespell suggested this change, and I based my decision to keep it based on [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not even listing "invokable" as an alternate spelling, and that "invocable" has about 3x the number of Google hits than "invokable". But sure, there is perhaps reason to consider "invokable" an acceptable alternative and keep it. I guess it depends on if you consider the word to be based on "invoke" with a suffix, or a latinized form, like "invocation". I'll wait a while for others to chime in, otherwise I'll revert the "invokable" changes. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) > @dfuch I have only updated files in `src`, so if the incorrect spelling is > tested, that test will now fail. I'm unfortunately not well versed in what > tests cover serviceability code. Can you suggest a suitable set of tests to > run? Folks from serviceability-dev will be more able to answer that - but I would suggest running tier2-tier3 as a precaution. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. All looks good to me, just the invokable which you might want to leave as is, unless there are other strong feelings. 8-) src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: > 36: jboolean pending; /* Is an invoke requested? */ > 37: jboolean started; /* Is an invoke happening? */ > 38: jboolean available;/* Is the thread in an invocable state? */ invocable could perhaps stay as invokable ? Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which we clearly don't want to change, and I see Internet dictionary evidence of invokable being acceptable. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 14:03:39 GMT, Daniel Fuchs wrote: >> I ran `codespell` on modules owned by the serviceability team >> (`java.instrument java.management.rmi java.management jdk.attach >> jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi >> jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and >> accepted those changes where it indeed discovered real typos. >> >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > LGTM. I spotted one fix in a exception message. I don't expect that there > will be tests depending on that, but it might still be a good idea to run the > serviceability tests to double check. Although I guess the test would have > had the same typo and would have been fixed too were it the case :-) @dfuch I have only updated files in `src`, so if the incorrect spelling is tested, that test will now fail. I'm unfortunately not well versed in what tests cover serviceability code. Can you suggest a suitable set of tests to run? And yes, ideally the tests should be spell checked as well. It's just that: 1) the product source is (imho) more important to begin with, 2) test comments are generally of a lower quality and more likely to contain more typos (imho), meaning even more work for me to publish a PR i believe is correct, and 3) the tests in the JDK are so intertwined and messy that I'm having a hard time understanding what groups to post reviews to. I could make one mega-PR touching the entire test code base, but I doubt it would be very popular. :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. LGTM. I spotted one fix in a exception message. I don't expect that there will be tests depending on that, but it might still be a good idea to run the serviceability tests to double check. Although I guess the test would have had the same typo and would have been fixed too were it the case :-) - PR: https://git.openjdk.java.net/jdk/pull/8334
