On Tue, 2 Nov 2021 16:30:56 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> This PR follows up one of the recent PRs, where I used a non-canonical > modifier order. Since the problem was noticed [^1], why not to address it en > masse? > > As far as I remember, the first mass-canonicalization of modifiers took place > in JDK-8136583 in 2015 [^2]. That change affected 1780 lines spanning 453 > files. Since then modifiers have become a bit loose, and it makes sense to > re-bless (using the JDK-8136583 terminology) them. > > This change was produced by running the below command followed by updating > the copyright years on the affected files where necessary: > > $ sh ./bin/blessed-modifier-order.sh src/java.base > > The resulting change is much smaller than that of 2015: 39 lines spanning 21 > files. > > [^1]: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-November/082987.html > (or https://github.com/openjdk/jdk/pull/6191#pullrequestreview-794333365) > [^2]: > http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035217.html > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) on > [core-libs-dev](mailto:core-libs-...@mail.openjdk.java.net):_ > > On 3/11/2021 9:26 pm, Pavel Rappo wrote: > > > On Tue, 2 Nov 2021 20:34:44 GMT, Martin Buchholz <martin at openjdk.org> > > wrote: > > > > > Pragmatically, fix the script to ignore those keywords on comment > > > > > lines. Learn Perl, its just a regular expression pattern match and > > > > > replace expression. > > > > > > > > > > > > I understand in principle how to modify that script to ignore doc > > > > comments. The thing I was referring to when said "btw, how would we do > > > > that?" was this: not all comment lines are prose. Some of those lines > > > > belong to snippets of code, which I guess you would also like to be > > > > properly formatted. > > > > > But having seen several reviewers be unmoved by the difference, the > > > > > real pragmatic view is to ignore the English. > > > > > > > > > > > > I'm sorry you feel that way. Would it be okay if I made it clear that > > > > those two words are not English adjectives but are special symbols that > > > > happen to use Latin script and originate from the English words they > > > > resemble? If so, I could enclose each of them in `{@code ... }`. If > > > > not, I could drop that particular change from this PR. > > > > > > > > > The blessed-modifier-order.sh script intentionally modifies comments, > > > with the hope of finding code snippets (it did!) > > > Probably I manually deleted the change to Object.java back in 2015, to > > > avoid the sort of controversy we're seeing now. > > > I don't have a strong feeling either way on changing that file. > > > I agree with @pavelrappo that script-generated changes should not be > > > mixed with manual changes. > > > I would also not update copyright years for such changes. > > > It's a feature of blessed-modifier-order.sh that all existing formatting > > > is perfectly preserved. > > > > > > One more thing. Please have a look at this other line in the same file; > > this line was there before the change > > https://github.com/openjdk/jdk/blob/465d350d0b3cac277a58b9f8ece196c1cde68e80/src/java.base/share/classes/java/lang/Object.java#L49 > > So before the change, the file was somewhat inconsistent. The change made > > it consistent. **If one is going to ever revert that controversial part of > > the change, please update both lines so that the file remains consistent.** > > Line 281 is (was!) consistent with line 277 because it is distinguishing a > synchronized "static method" from a synchronized "instance method". There was > no need to make any change to line 281 because of the blessed-order of > modifiers as defined for method declarations! This text is just prose. Now > for consistency you should change line 277 to refer to a "non-static > synchronized method" (as "instance synchronized method" would be truly awful). Thanks, David. You've provided a clear and convincing argument, and I can see the inconsistency I introduced. I can revert that particular piece back if you think that it would be appropriate. That said, this line will have to be filtered out every time the script is run. I could probably provide a more involved script that harnesses the power of AST (com.sun.source.doctree) to try to filter out prose, but it would be impossible to beat the simplicity of the current script; and simplicity is also important. > Line 49 places "static synchronized" in code font, implying that it is > referring to the actual method modifiers and as such using the blessed order > is appropriate. Line 49 does not need to be "consistent" with line 281. If > line 49 used a normal font so the words "static" and "synchronized" were just > prose then either order would be perfectly fine in terms of English language, > but then you could make a case for having it be consistent with line 281. I've been always having hard time with modifiers being not enclosed in `@code` in the first place; they are barely English words. Is there really a semantic difference between L49 and L281 such that it warrants the use of `@code` in the former but not in the latter case? Does `synchornized` or `static` in L281 refer to anything other than the like-named Java modifiers? ------------- PR: https://git.openjdk.java.net/jdk/pull/6213
