[PATCH] D69354: Make coding standards document more inclusive

Tue Nov 19 10:24:43 PST 2019

gribozavr2 marked 12 inline comments as done.
gribozavr2 added inline comments.

================
Comment at: llvm/docs/CodingStandards.rst:79
+<https://github.com/llvm/llvm-project/tree/master/llvm/include/llvm/ADT>`_)
+providing func tionality missing from the standard library for which there are
+standard interfaces or active work on adding standard interfaces will often be
----------------
bmcreusillet wrote:
> There seem to be a spurious space in "func tionality"
> 
> Also the whole sentence is somewhat long, I would add commas for instance before "providing" and before "will often" to ease readability.
> 
Thanks, removed the extra space and rewrote the sentence.

================
Comment at: llvm/docs/CodingStandards.rst:170

-Classes are one fundamental part of a good object oriented design.  As such, a
+Classes are a fundamental part of a object-oriented design.  As such, a
 class definition should have a comment block that explains what the class is
----------------
hfinkel wrote:
> bmcreusillet wrote:
> > a object-oriented -> an object oriented?
> Should be:
> 
>   an object-oriented design
> 
> (compound adjectives get a dash)
> 
s/a/an/ done.

================
Comment at: llvm/docs/CodingStandards.rst:184
 Good things to talk about here are what happens when something unexpected
-happens: does the method return null?  Abort?  Format your hard disk?
+happens: does the method return null?

----------------
bmcreusillet wrote:
> I would turn this into: "for instance, does the method return null?", otherwise it may seem the only possibility. 
Done.

================
Comment at: llvm/docs/CodingStandards.rst:294

-  // In Something.cpp:
+  // In Example.cpp:

----------------
bmcreusillet wrote:
> The "In" does not seem consistent with the C example
Made them consistent.

================
Comment at: llvm/docs/CodingStandards.rst:308

-Consider:
+  // In Example.cpp:

----------------
bmcreusillet wrote:
> same remark about "In"
Made them consistent.

================
Comment at: llvm/docs/CodingStandards.rst:426

 With C++11, there are significantly more uses of braced lists to perform
+initialization. For example, they can be used to construct aggregate
----------------
bmcreusillet wrote:
> A remark not related to the changes themselves:
> 
> "With C++11": Shouldn't it be "From C++11" or something similar?
Changed to "starting from c++11".

================
Comment at: llvm/docs/CodingStandards.rst:89

-Supported C++14 Language and Library Features
----------------------------------------------
-
-While LLVM, Clang, and LLD use C++14, not all features are available in all of
-the toolchains which we support. The set of features supported for use in LLVM
-is the intersection of those supported in the minimum requirements described
-in the :doc:`GettingStarted` page, section `Software`.
-The ultimate definition of this set is what build bots with those respective
-toolchains accept. Don't argue with the build bots. However, we have some
-guidance below to help you know what to expect.
-
-Each toolchain provides a good reference for what it accepts:
-
-* Clang: https://clang.llvm.org/cxx_status.html
-* GCC: https://gcc.gnu.org/projects/cxx-status.html#cxx14
-* MSVC: https://msdn.microsoft.com/en-us/library/hh567368.aspx
-
-Other Languages
----------------
+Guidelines for Go code
+----------------------
----------------
arsenm wrote:
> arsenm wrote:
> > New empty section?
> Nevermind, the section below was renamed and mostly shifted out of the diff view
Marking as resolved.

================
Comment at: llvm/docs/CodingStandards.rst:507
+
+Globals in different source files are initialized in `arbitrary order
+<https://yosefk.com/c++fqa/ctors.html#fqa-10.12>`.
----------------
bmcreusillet wrote:
> This sentence does not appear to be properly linked to the preceding nor to the following one.
> 
> I feel a small explanation of the consequences in this sentence could be helpful. Also beginning the next sentence with "Moreover" or something appropriate.
Added "making the code more difficult to reason about" to this sentence.

================
Comment at: llvm/docs/CodingStandards.rst:510
+
+Static constructors have have negative impact on launch time of programs that
+use LLVM as a library. We would really like for there to be zero cost for
----------------
bmcreusillet wrote:
> s/have have/have/
Done.

================
Comment at: llvm/docs/CodingStandards.rst:559

 In C++11 there is a "generalized initialization syntax" which allows calling
 constructors using braced initializer lists. Do not use these to call
----------------
bmcreusillet wrote:
> Same remarks as before about the reference to C++11 ?
Changed to "starting from c++11".

================
Comment at: llvm/docs/CodingStandards.rst:941
+Instead of this sort of loop, we prefer to use a predicate function (which may
+be `static`_) that uses `early exits`_.  We prefer the code to be structured
+like this:
----------------
bmcreusillet wrote:
> "we prefer" is repeated at a short interval. Maybe change the second one into "The code should instead be structured like this:"
Removed the second sentence altogether, it does not say anything new.

================
Comment at: llvm/docs/CodingStandards.rst:1356
 The reason for doing this is not completely arbitrary.  This style makes control
-flow operators stand out more, and makes expressions flow better. The function
-call operator binds very tightly as a postfix operator.  Putting a space after a
-function name (as in the last example) makes it appear that the code might bind
-the arguments of the left-hand-side of a binary operator with the argument list
-of a function and the name of the right side.  More specifically, it is easy to
-misread the "``A``" example as:
-
-.. code-block:: c++
-
-  A = foo ((42, 92) + bar) (X);
-
-when skimming through the code.  By avoiding a space in a function, we avoid
-this misinterpretation.
+flow operators stand out more, and makes expressions flow better.

----------------
hfinkel wrote:
> control-flow operators
I don't think this phrase is written with a dash.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69354/new/

https://reviews.llvm.org/D69354