RFC: 8247976: Update HotSpot Style Guide for C++14 adoption

[Stefan Karlsson]

Hi Kim,

Thanks for working on updating this document. I think this mostly looks 
good, but do have a few comments.

  *

    Macro names usually use upper-case with words separated by a single
    underscore. However, there are a significant number of lowercase
    macros. Adding to that set is discouraged.

This is new line from the old style guide. Personally I find upper-case 
macros very noise and grabs a lot of the attention of the code. For most 
macros that's OK, but if a macro is prevalent in the source code, like 
assert, guarantee, log_info, offset_of, maybe even the recently 
discussed ENABLE_IF/enable_if, then I think it makes sense to *NOT* use 
upper-case names.

  *

    Use functions from globalDefinitions.hpp when performing bitwise
    operations on integers. Do not code directly as C operators, unless
    they are extremely simple.
    (Examples:|round_to|,|is_power_of_2|,|exact_log2|.)

Over the years, we've seen problems with the intptr_t bit manipulation 
functions in globalDefinitions.hpp. We have been slowly fixing these 
problems and moving some of them away into other headers like align.hpp 
and powerOfTwo.hpp. The bit-manipulation functions left in 
globalDefinitions.hpp still use intptr_t and I don't think we should 
encourage people to using them before they have been rewritten, and 
fully tested with all integer types.

round_to has been removed in favor of the align_up function.


  *

    Put a member function|FooBar::bang|into the same file that
    defined|FooBar|, or its associated *.cpp file.

I don't think we should promote putting implementation in our header files.

  *

    Implement classes as if expecting rough usage by clients. Check for
    incorrect usage of a class
    using|assert(...)|,|guarantee(...)|,|ShouldNotReachHere()|and
    comments wherever needed. Performance is almost never a reason to
    omit asserts.

I know that the Compiler team had a push to replace ShouldNotReachHere() 
with fatal("descriptive message"). It would be good to have a common 
guidance w.r.t. that. Could be done as a follow-up task.


  *

    When you must defactor to optimize, preserve as much structure as
    possible. If you must hand-inline some name, label the local copy
    with the original name.

Is defactor a real word? Could this be rewritten to skip that word?


  *

    Use One-True-Brace-Style. The opening brace for a function or class
    is normally at the end of the line; it is sometimes moved to the
    beginning of the next line for emphasis. Substatements are enclosed
    in braces, even if there is only a single statement. Extremely
    simple one-line statements may drop braces around a substatement.

Why not just skip the last sentence? People interpret extremely simple 
one-liners differently and this sentence just adds a loop-hole for those 
that like to write if-statements without braces. This could be 
considered an extremely simple one-liner:

   int append(const E& elem) {
     if (this->_len == this->_max) grow(this->_len);
     int idx = this->_len++;
     this->_data[idx] = elem;
     return idx;
   }

but I don't think we should be writing code like this anymore.

  *

    Use extra parentheses in expressions whenever operator precedence
    seems doubtful. Always use parentheses in shift/mask expressions
    (|<<|,|&|,|||,|~|). Don’t add whitespace immediately inside parentheses.

Isn't it a bit odd to mention ~ here? It precedes almost all operators, 
so either they are needed or they are not. There's not really a style 
choice.


  *

    Type names and global names should use mixed-case with the first
    letter of each word capitalized (|FooBar|). Embedded abbreviations
    in otherwise mixed-case names are usually capitalized entirely
    rather than being treated as a single word with only the initial
    letter capitalized, e.g. “HTML” rather than “Html”.

Could this be split into two separate bullets?

I think the mixed-case guide makes sense, but I'm not a fan when it's 
used dogmatically. I would much rather allow us to write atomic<int> 
instead of the longer more noisier AtomicValue<int>. And I know that 
this rule is used to argument for the latter.


  *

    Constant names may be upper-case or mixed-case, according to
    historical necessity. (Note: There are many examples of constants
    with lowercase names.)

The "according to historical necessity" makes it unclear if there's a 
preferred way for new code. If there's no preferred way out of the two, 
maybe drop that part?

  *

    Use|bool|for boolean values. Do not use ints or pointers as
    (implicit) booleans with|&&|,||||,|if|,|while|. Instead, compare
    explicitly, i.e.|if (x != 0)|or|if (ptr != nullptr)|, etc. Do not
    use declarations in/condition/forms, i.e. don’t use|if (T v = value)
    { ... }|.

The last part is a new addition to the style guide. We use it for some 
iterators:

for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thr = jtiwh.next(); ) {

this is useful to limit the scope of the instance. I don't think we 
should discourage this in the style guide.


- "Some features are discussed their own subsection"

Missing an "in"?

StefanK

On 2020-06-23 21:32, Kim Barrett wrote:
> Please review / comment on this proposed update to the HotSpot Style
> Guide. This is part of the work for JDK-8208089: JEP 347: Adopt C++14
> Language Features in the JDK.
>
> This update includes additions for initial C++14 adoption. It has also
> been significantly refactored and modified from the current wiki text.
> There have long been complaints that what's on the wiki hasn't kept up
> with actual usage. I've also added more explanation and rationale for
> some parts, based on (sometimes repeated) discussions in various
> forums. (Note that I might not always entirely agree with some
> choices, but I've tried to fairly and accurately record what I think
> is the rationale. Feel free to offer corrections.)
>
> For convenience of review, especially for incremental updates based on
> feedback, I'm publishing the updated document as a webrev. Once
> reviewed, the finished text can be used to update the wiki, unless we
> decide it should be homed somewhere else.
>
> We don't currently have a process for making updates to this document
> (one reason for it falling out of date). This update includes a
> proposal for such a process, but that process may need approvals from
> elsewhere, and there's the whole bootstrapping question. We'll see how
> this goes; maybe there won't be any complaints and it's done. That's
> likely wishful thinking.
>
> To see the html-formatted version you'll probably need to download and
> view it locally; cr.openjdk.java.net doesn't seem to like serving it
> from the webrev.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8247976
>
> Webrev:
> https://cr.openjdk.java.net/~kbarrett/8247976/hotspot-style.00/
>
>