[PATCH] D69354: Make coding standards document more inclusive
BĂ©atrice Creusillet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 30 01:35:33 PDT 2019
bmcreusillet 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
----------------
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.
================
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
----------------
a object-oriented -> an object oriented?
================
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?
----------------
I would turn this into: "for instance, does the method return null?", otherwise it may seem the only possibility.
================
Comment at: llvm/docs/CodingStandards.rst:294
- // In Something.cpp:
+ // In Example.cpp:
----------------
The "In" does not seem consistent with the C example
================
Comment at: llvm/docs/CodingStandards.rst:308
-Consider:
+ // In Example.cpp:
----------------
same remark about "In"
================
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
----------------
A remark not related to the changes themselves:
"With C++11": Shouldn't it be "From C++11" or something similar?
================
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>`.
----------------
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.
================
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
----------------
s/have have/have/
================
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
----------------
Same remarks as before about the reference to 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:
----------------
"we prefer" is repeated at a short interval. Maybe change the second one into "The code should instead be structured like this:"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69354/new/
https://reviews.llvm.org/D69354
More information about the llvm-commits
mailing list