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

[Kim Barrett]

> On Jun 24, 2020, at 3:24 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi Kim,
> 
> Thanks for working on updating this document. I think this mostly looks good, but do have a few comments.

Thanks for looking at it.

Some of your comments are, I think, suggestions for substantive style
changes. I tried not to make too many of those in this revision, other
than as related to features from the new Standards. And at least some
of your suggestions seem like they would be worthy of their own
(possibly significant) discussion.

(I did quietly drop "Be sparing with templates." and "Introduce new
templates only by Act of Congress.". I also quietly dropped mention of
+Verbose, since we should be using UL.)

I'm hoping this refresh will be part of making this a living document
that we'll change to reflect our needs and actual usage. So I'd like
to defer such points

[Note: Stefan and I discussed this offline, and I think we're in
agreement on most of the points below.]

> *
> 
>   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.

This *is* a place where I went beyond editorial cleanup of the
existing text. I will take this out and bring it up for discussion
later.

> *
> 
>   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.

This is directly from the original document. I understand the intent,
even though the execution has been lacking, so didn't want to quietly
remove it. I agree that the current set of bit manipulation functions
in globalDefinitions.hpp have problems. Besides the types being used,
they also aren't constexpr, making them useless in many common contexts.

I think revisiting this area with new language features in hand would
be worth considering.

> round_to has been removed in favor of the align_up function.

Changed to align_up and also added “and related files” after globalDefinitions.hpp,
since we’ve been moving some of these elsewhere.

> *
> 
>   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.

And we also have:

* Do not put non-trivial function implementations in .hpp files. If the
implementation depends on other .hpp files, put it in a .cpp or a
.inline.hpp file.

I've changed it to "associated *.inline.hpp or *.cpp"?

But I don't think the trend toward lots of .inline.hpp files is
necessarily good either, especially since I think we man not be using
them properly. (We had a discussion about this some time ago.) But
that can be a discussion for later.

> *
> 
>   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.

Yes, that should be a followup.

> *
> 
>   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?

I think "defactor" is a word, and this was copied directly from the
original text. I'd rather not try to rewrite it at this time.

> *
> 
>   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.

That's a rephrasing of the original, which said:

  Use braces around substatements. (Relaxable for extremely simple
  substatements on the same line.)

I think this should be a followup.

> *
> 
>   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.

Good point.  That was in the original text, but you are right that
it's useless here.  Removed.

> *
> 
>   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?

Split.  The all-caps style for abbreviation is an addition, not
present in the original text.  I added it based on observation and a
discussion I observed in a review thread a while ago.

> 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.

I think this should be a followup. 

> *
> 
>   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?

That phrasing is from the original text.  I *think* it probably means
one should follow the nearby style, and acknowledges that there's been
significant variation on this point.

This is something I'd like to see be more prescriptive; I feel like I
never know how to name constants in such a way that I won't get review
feedback requesting something different.

> *
> 
>   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.

I think that's not a new addition, just making explicit something
which is already implicit. Using that feature is clearly using an
integer or pointer as a boolean, or using a boolean conversion
operator (and without C++11 explicit conversions those are either
implicit (nearly always a bad idea) or using the safe-bool idiom,
which is a pain to write and looks really strange to someone not
already familiar with it).

So I think permitting it would be a substantive change.

I made it explicit to call attention to it, as I'd like to see this
changed.  While I tried to avoid making substantive changes, that
doesn't mean there aren't some leading questions.

> - "Some features are discussed their own subsection"
> 
> Missing an "in”?

Fixed.

I also have a couple of changes suggested by John Rose offline, as well
as a couple of nomenclature corrections that I’d previously missed.  I’m
going to wait a bit before sending an update, to see what other comments
might come in.