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

[Stefan Karlsson]

Sounds good to me.

Thanks,
StefanK

On 2020-06-24 16:44, Kim Barrett wrote:
>> 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.
>