- Rename anonymous namespace section and rework it to
cover visibility more broadly.
- Add language suggesting restricting visibility as much as
possible, using various C++ facilities.
---------
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
It was previously present in the inline code block and did not work as a
hyperlink.
Reviewed By: yassingh
Differential Revision: https://reviews.llvm.org/D153061
This reverts commit ee9ccb11036ac46ff9b9f3fa2d459d9d2bce5509.
See https://reviews.llvm.org/D140585#4019417 and following.
Multiple people requested a revert of this change pending further
discussion.
We currently suggest that users not use an else clause after a return
statement in a prior if branch. e.g.,
if (foo)
return 1;
else // Should remove this else clause
return 10;
however, this suggestion is incorrect for a constexpr if statement
because one of the two branches will be a discarded statement and thus
can impact template instantiation behavior. This updates the coding
standard to make it clear that it's okay to have a return after an else
in a constexpr if statement.
I think this is an NFC change to the intent of the rule, which is why
I've not started an RFC for the changes.
Differential Revision: https://reviews.llvm.org/D132232
Also make the soft toolchain requirements hard. This allows
us to use C++17 features in LLVM now.
If we find patterns with C++17 that improve readability
it should be recommended in the coding standards.
Reviewed By: jhenderson, cor3ntin, MaskRay
Differential Revision: https://reviews.llvm.org/D130689
While working on a clang-format option RemoveBracesLLVM that removes
braces following the guideline, we were unsure about what to do with
the braces of do-while loops. The ratio of using to omitting the
braces is about 4:1 in the llvm-project source, so it will help to
add an example to the guideline.
Also cleans up the original examples including making the nested if
example more targeted on avoiding potential dangling else situations.
Differential Revision: https://reviews.llvm.org/D126512
There are many more instances of this pattern, but I chose to limit this change to .rst files (docs), anything in libcxx/include, and string literals. These have the highest chance of being seen by end users.
Reviewed By: #libc, Mordante, martong, ldionne
Differential Revision: https://reviews.llvm.org/D124708
The Coding Guidelines specify that the ending brace of a
namespace shall have a comment like:
} // end namespace clang
However the majority of the code uses a different style:
} // namespace clang
Indeed:
$ git grep '// end' | wc -l
6724
$ git grep '// namespace' | wc -l
14348
Besides, this is the style enforced automatically by clang-format,
via the FixNamespaceComments option.
Having inconsistencies between the Coding Guidelines and the
code/tooling creates confusion, can lead to bikeshedding during
reviews and overall delays merging code. Therefore, update the
guidelines to reflect current usage. Updating legacy code to the
new standard should be done in a separate patch, if wanted.
Reviewed By: jyknight
Differential Revision: https://reviews.llvm.org/D115115
No longer rely on an external tool to build the llvm component layout.
Instead, leverage the existing `add_llvm_componentlibrary` cmake function and
introduce `add_llvm_component_group` to accurately describe component behavior.
These function store extra properties in the created targets. These properties
are processed once all components are defined to resolve library dependencies
and produce the header expected by llvm-config.
Differential Revision: https://reviews.llvm.org/D90848
Like most readability rules, it isn't absolute and there is a matter of taste
to it. I think more recent part of the project may be more consistent in the
current application of the guideline. I suspect sources like
mlir/lib/Dialect/StandardOps/IR/Ops.cpp may be examples of this at the moment.
Differential Revision: https://reviews.llvm.org/D82594
Summary:
As per disscussion in D83351, using `for_each` is potentially confusing,
at least in regards to inconsistent style (there's less than 100 `for_each`
usages in LLVM, but ~100.000 `for` range-based loops
Therefore, it should be avoided.
Reviewers: dblaikie, nickdesaulniers
Reviewed By: dblaikie, nickdesaulniers
Subscribers: hubert.reinterpretcast, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D83431
This is a rule that seems to have been enforced for the better part of
the decade, so we should document it for new contributors.
Differential Revision: https://reviews.llvm.org/D80947
Just two paragraphs above it says:
"If the compiler does not support this [skipping code generation for a particular branch], it will fall back
to the "abort" implementation."
And that actually correctly describes llvm_unreachable implementation.
Differential Revision: https://reviews.llvm.org/D81130
There has been some ongoing confusion regarding when to use `llvm_unreachable`
which this patch attempts to address. Specifically, the confusion has been
around whether `llvm_unreachable` is intended to mark only unreachable code
paths that the compiler cannot determine itself or to mark a code path which is
unconditionally a bug to reach. Based on email and IRC discussions, it sounds
like "unconditional bug to reach" is the consensus.
There is prior art for this in the code base itself, and a recent
example of this here: c45f8d49897f
This came up in discussion on this review where @maskray was going the
opposite direction:
https://reviews.llvm.org/D68772
Given that there is disagreement, we should make a choice and document
it.
Thanks to John McCall for the precise wording.
Reviewed By: MaskRay, rjmccall
Differential Revision: https://reviews.llvm.org/D74515
The existing wording leaves it unclear if C++ standard library data
structures should be preferred over custom LLVM ones, e.g., SmallVector,
even though common practice seems clear on the issue. This change makes
the wording more explicit and aligns it better with the code base.
Some motivating statistics:
```
ag SmallVector llvm/lib/ | wc
8846 40306 901421
ag 'std::vector' llvm/lib/ | wc
2123 8990 214482
ag SmallVector clang/lib/ | wc
3023 13824 281691
ag 'std::vector' clang/lib/ | wc
719 2914 72817
```
Differential Revision: https://reviews.llvm.org/D74340
This is a bit awkward in a handful of places where we didn't even have
an instruction and now we have to see if we can build one. But on the
whole, this seems like a win and at worst a reasonable cost for removing
`TerminatorInst`.
All of this is part of the removal of `TerminatorInst` from the
`Instruction` type hierarchy.
llvm-svn: 340701
Clarify that you should not introduce trailing whitespace when making a commit and that you should not remove trailing whitespace that's unrelated to code you are changing or are about to change. Then clarified the developer policy around what is considered an obvious whitespace commit.
llvm-svn: 339455
(I suppose these two pieces could be separated - but seemed related
enough)
As discussed on llvm-dev, this documents the general expectation of how
library layering should be handled. There are a few existing cases where
these constraints are not met, but as with most style guide things -
this is forward looking and provides guidance when cleaning up existing
code, it doesn't immediately require that all previous code be cleaned
up to match. (see: naming conventions, etc)
Differential Revision: https://reviews.llvm.org/D42771
llvm-svn: 324004
