This is an archive of the discontinued LLVM Phabricator instance.

Make coding standards document more inclusive
ClosedPublic

Authored by gribozavr on Oct 23 2019, 12:48 PM.

Details

Summary

Patch by Doug Gregor, Tres Popp, and Dmitri Gribenko.

Diff Detail

Event Timeline

gribozavr created this revision.Oct 23 2019, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2019, 12:48 PM

A few minor further tweaks.

llvm/docs/CodingStandards.rst
56

Polly? Maybe its not important to list everything here.

Maybe this should say "Unless otherwise documented, LLVM subprojects are written ..."

60–61

"*Those* features are the intersection of *those* supported ..."

Maybe: "Nevertheless, we restrict ourselves to the intersection of features which are available..." and then "We describe our host compiler support in the GettingStarted page..."?

356

s/reasonably// ?

459

I think we can make this even better... Maybe:

"Compiler warnings are often useful and will help improve the code. Those that are not ..."

491–494

Delete the entire sentence from "These two language features ... is never used for a class.". I don't think it is phrased in an objective or good way IMO.

It might be possible to rephrase it, but honestly I don't think its that useful and so i'd also probable delete the subsequent sentence and keep going as far as needed. I don't think we need a lot of detailed discussion here.

gribozavr marked 8 inline comments as done.

Addressed review comments

llvm/docs/CodingStandards.rst
56

Changed to a blanket rule.

60–61

Changed to a parenthetical.

Nevertheless, we restrict ourselves to features which are available in the
major toolchains supported as host compilers (see :doc:GettingStarted page,
section Software).

356

Done.

459

Done.

491–494

Removed this sentence and other discussion of dynamic_cast.

gribozavr edited the summary of this revision. (Show Details)Oct 24 2019, 12:33 PM
bmcreusillet removed a subscriber: bmcreusillet.

Can this get merged now?

arsenm added a subscriber: arsenm.Oct 29 2019, 1:34 PM
arsenm added inline comments.
llvm/docs/CodingStandards.rst
89

New empty section?

arsenm added inline comments.Oct 29 2019, 1:35 PM
llvm/docs/CodingStandards.rst
89

Nevermind, the section below was renamed and mostly shifted out of the diff view

bmcreusillet added inline comments.Oct 30 2019, 1:31 AM
llvm/docs/CodingStandards.rst
79

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.

170

a object-oriented -> an object oriented?

184

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

294

The "In" does not seem consistent with the C example

308

same remark about "In"

426

A remark not related to the changes themselves:

"With C++11": Shouldn't it be "From C++11" or something similar?

507

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.

510

s/have have/have/

559

Same remarks as before about the reference to C++11 ?

941

"we prefer" is repeated at a short interval. Maybe change the second one into "The code should instead be structured like this:"

hfinkel added inline comments.
llvm/docs/CodingStandards.rst
170

Should be:

an object-oriented design

(compound adjectives get a dash)

1356

control-flow operators

gribozavr2 marked 12 inline comments as done.Nov 19 2019, 10:18 AM
gribozavr2 added a subscriber: gribozavr2.
gribozavr2 added inline comments.
llvm/docs/CodingStandards.rst
79

Thanks, removed the extra space and rewrote the sentence.

89

Marking as resolved.

170

s/a/an/ done.

184

Done.

294

Made them consistent.

308

Made them consistent.

426

Changed to "starting from c++11".

507

Added "making the code more difficult to reason about" to this sentence.

510

Done.

559

Changed to "starting from c++11".

941

Removed the second sentence altogether, it does not say anything new.

1356

I don't think this phrase is written with a dash.

Addressed review comments.

@chandlerc has LGTM'ed this patch over chat.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 21 2019, 4:42 AM
This revision was automatically updated to reflect the committed changes.