This reverts commit cc56c66f27.
Fixed a bad assertion, the target of a UsingShadowDecl must not have
*local* qualifiers, but it can be a typedef whose underlying type is qualified.
Currently there's no way to find the UsingDecl that a typeloc found its
underlying type through. Compare to DeclRefExpr::getFoundDecl().
Design decisions:
- a sugar type, as there are many contexts this type of use may appear in
- UsingType is a leaf like TypedefType, the underlying type has no TypeLoc
- not unified with UnresolvedUsingType: a single name is appealing,
but being sometimes-sugar is often fiddly.
- not unified with TypedefType: the UsingShadowDecl is not a TypedefNameDecl or
even a TypeDecl, and users think of these differently.
- does not cover other rarer aliases like objc @compatibility_alias,
in order to be have a concrete API that's easy to understand.
- implicitly desugared by the hasDeclaration ASTMatcher, to avoid
breaking existing patterns and following the precedent of ElaboratedType.
Scope:
- This does not cover types associated with template names introduced by
using declarations. A future patch should introduce a sugar TemplateName
variant for this. (CTAD deduced types fall under this)
- There are enough AST matchers to fix the in-tree clang-tidy tests and
probably any other matchers, though more may be useful later.
Caveats:
- This changes a fairly common pattern in the AST people may depend on matching.
Previously, typeLoc(loc(recordType())) matched whether a struct was
referred to by its original scope or introduced via using-decl.
Now, the using-decl case is not matched, and needs a separate matcher.
This is similar to the case of typedefs but nevertheless both adds
complexity and breaks existing code.
Differential Revision: https://reviews.llvm.org/D114251
This change adds an option to disable warnings from the
cppcoreguidelines-narrowing-conversions check on integer to floating-
point conversions which may be narrowing.
An example of a case where this might be useful:
```
std::vector<double> v = {1, 2, 3, 4};
double mean = std::accumulate(v.cbegin(), v.cend(), 0.0) / v.size();
```
The conversion from std::size_t to double is technically narrowing on
64-bit systems, but v almost certainly does not have enough elements
for this to be a problem.
This option would allow the cppcoreguidelines-narrowing-conversions
check to be enabled on codebases which might otherwise turn it off
because of cases like the above.
The purpose of this checker is to flag a missing throw keyword, and does so by checking for the construction of an exception class that is then unused.
This works great except that placement new expressions are also flagged as those lead to the construction of an object as well, even though they are not temporary (as that is dependent on the storage).
This patch fixes the issue by exempting the match if it is within a placement-new.
Fixes https://github.com/llvm/llvm-project/issues/51939
Differential Revision: https://reviews.llvm.org/D115576
This change applies two fixes to the abseil-cleanup-ctad check. It uses hasSingleDecl() to ensure only declStmt()s with one varDecl() are matched (leaving compount declStmt()s unchanged). It also addresses a bug in the handling of comments that surround the absl::MakeCleanup() calls by switching to the callArgs() combinator from Clang Transformer.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D115452
This commit improves the fix-its of modernize-pass-by-value by
no longer proposing partial fixes. In the presence of using/typedef,
we failed to rewrite the function signature but still adjusted the
function body. This led to incorrect, partial fix-its. Instead, the
check now simply doesn't offer any fixes at all in such a situation.
Currently it's hidden inside ClangTidyDiagnosticConsumer,
so it's hard to know it exists.
Given that there are multiple uses of globs in clang-tidy,
it makes sense to have these classes publicly available
for other use cases that might benefit from it.
Also, add unit test by converting the existing tests
for GlobList into typed tests.
Reviewed By: salman-javed-nz
Differential Revision: https://reviews.llvm.org/D113422
Using XCTAssertEqual on NSString* objects is almost always wrong.
Unfortunately, we have seen a lot of tests doing this and reyling on pointer equality for strings with the same values (which happens to work sometimes - depending on the linker, but this assumption is not guaranteed by the language)
These fixes would make tests less brittle.
Differential Revision: https://reviews.llvm.org/D114975
This patch parameterizes the clang-tidy diagnostic consumer with a boolean that
controls whether to honor NOLINTBEGIN/NOLINTEND blocks. The current support for
scanning these blocks is very costly -- O(n*m) in the size of files (n) and
number of diagnostics found (m), with a large constant factor. So, the patch
allows clients to disable it.
Future patches should make the feature more efficient, but this will mitigate in
the interim.
Differential Revision: https://reviews.llvm.org/D114981
Checks for various ways that the `const CharT*` constructor of `std::basic_string_view` can be passed a null argument and replaces them with the default constructor in most cases. For the comparison operators, braced initializer list does not compile so instead a call to `.empty()` or the empty string literal are used, where appropriate.
This prevents code from invoking behavior which is unconditionally undefined. The single-argument `const CharT*` constructor does not check for the null case before dereferencing its input. The standard is slated to add an explicitly-deleted overload to catch some of these cases: wg21.link/p2166
https://reviews.llvm.org/D114823 is a companion change to prevent duplicate warnings from the `bugprone-string-constructor` check.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D113148
Fixes PR#47614. Deduction guides, implicit or user-defined, look like
function declarations in the AST. They aren't really functions, though,
and they always have a trailing return type, so it doesn't make sense
to issue this warning for them.
Up until now, all references to `errno` were marked with `NOLINT`, since
it was technically calling an external function. This fixes the lint
rules so that `errno`, as well as `malloc`, `calloc`, `realloc`, and
`free` are all allowed to be called as external functions. All of the
relevant `NOLINT` comments have been removed, and the documentation has
been updated.
Reviewed By: sivachandra, lntue, aaron.ballman
Differential Revision: https://reviews.llvm.org/D113946
The google-readability-casting check is meant to be on par
with cpplint's readability/casting check, according to the
documentation. However it currently does not diagnose
functional casts, like:
float x = 1.5F;
int y = int(x);
This is detected by cpplint, however, and the guidelines
are clear that such a cast is only allowed when the type
is a class type (constructor call):
> You may use cast formats like `T(x)` only when `T` is a class type.
Therefore, update the clang-tidy check to check this
case.
Differential Revision: https://reviews.llvm.org/D114427
Fixes https://bugs.llvm.org/show_bug.cgi?id=48613.
llvm-header-guard is suggesting header guards with leading underscores
if the header file path begins with a '/' or similar special character.
Only reserved identifiers should begin with an underscore.
Differential Revision: https://reviews.llvm.org/D114149
Bitfields are special. Due to integral promotion [conv.prom/5] bitfield
member access expressions are frequently wrapped by an implicit cast to
`int` if that type can represent all the values of the bitfield.
Consider these examples:
struct SmallBitfield { unsigned int id : 4; };
x.id & 1; (case-1)
x.id & 1u; (case-2)
x.id << 1u; (case-3)
(unsigned)x.id << 1; (case-4)
Due to the promotion rules, we would get a warning for case-1. It's
debatable how useful this is, but the user at least has a convenient way
of //fixing// it by adding the `u` unsigned-suffix to the literal as
demonstrated by case-2. However, this won't work for shift operators like
the one in case-3. In case of a normal binary operator, both operands
contribute to the result type. However, the type of the shift expression is
the promoted type of the left operand. One could still suppress this
superfluous warning by explicitly casting the bitfield member access as
case-4 demonstrates, but why? The compiler already knew that the value from
the member access should safely fit into an `int`, why do we have this
warning in the first place? So, hereby we suppress this specific scenario,
when a bitfield's value is implicitly cast to int (likely due to integral
promotion).
Note that the bitshift operation might invoke unspecified/undefined
behavior, but that's another topic, this checker is about detecting
conversion-related defects.
Example AST for `x.id << 1`:
BinaryOperator 'int' '<<'
|-ImplicitCastExpr 'int' <IntegralCast>
| `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
| `-MemberExpr 'unsigned int' lvalue bitfield .id
| `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
`-IntegerLiteral 'int' 1
Reviewed By: courbet
Differential Revision: https://reviews.llvm.org/D114105
Invalid SourceRanges can occur generally if the code does not compile,
thus we expect clang error diagnostics.
Unlike `clang`, `clang-tidy` did not swallow invalid source ranges, but
tried to highlight them, and blow various assertions.
The following two examples produce invalid source ranges, but this is
not a complete list:
void test(x); // error: unknown type name 'x'
struct Foo {
member; // error: C++ requires a type specifier for all declarations
};
Thanks @whisperity helping me fix this.
Reviewed-By: xazax.hun
Differential Revision: https://reviews.llvm.org/D114254
The `cppcoreguidelines-virtual-base-class-destructor` checker crashed on
this example:
#define DECLARE(CLASS) \
class CLASS { \
protected: \
virtual ~CLASS(); \
}
DECLARE(Foo); // no-crash
The checker will hit the following assertion:
clang-tidy: llvm/include/llvm/ADT/Optional.h:196: T &llvm::optional_detail::OptionalStorage<clang::Token, true>::getValue() & [T = clang::Token]: Assertion `hasVal' failed."
It turns out, `Lexer::findNextToken()` returned `llvm::None` within the
`getVirtualKeywordRange()` function when the `VirtualEndLoc`
SourceLocation represents a macro expansion.
To prevent this from happening, I decided to propagate the `llvm::None`
further up and only create the removal of `virtual` if the
`getVirtualKeywordRange()` succeeds.
I considered an alternative fix for this issue:
I could have checked the `Destructor.getLocation().isMacroID()` before
doing any Fixit calculation inside the `check()` function.
In contrast to this approach my patch will preserve the diagnostics and
drop the fixits only if it would have crashed.
Reviewed By: whisperity
Differential Revision: https://reviews.llvm.org/D113558
`isConstRefReturningMethodCall` should be considering
`CXXOperatorCallExpr` in addition to `CXXMemberCallExpr`. Clang considers
these to be distinct (`CXXOperatorCallExpr` derives from `CallExpr`, not
`CXXMemberCallExpr`), but we don't care in the context of this
check.
This is important because of
`std::vector<Expensive>::operator[](size_t) const`.
Differential Revision: https://reviews.llvm.org/D114249
Overriding methods should not get a readability-identifier-naming
warning because the issue can only be fixed in the base class; but the
current check for whether a method is overriding does not take the
override attribute into account.
Differential Revision: https://reviews.llvm.org/D113830
The overload shouldSuppressDiagnostic seems unnecessary, and it is only
used in clangd.
This patch removes it and use the real one (suppression diagnostics are
discarded in clangd at the moment).
Fixes https://github.com/clangd/clangd/issues/929
Differential Revision: https://reviews.llvm.org/D113999
Fixes PR#52400. The tests for bugprone-throw-keyword-missing actually
already contain exceptions as class members, but not as members with
initializers, which was probably just an oversight.
This implements the following changes:
* AutoType retains sugared deduced-as-type.
* Template argument deduction machinery analyses the sugared type all the way
down. It would previously lose the sugar on first recursion.
* Undeduced AutoType will be properly canonicalized, including the constraint
template arguments.
* Remove the decltype node created from the decltype(auto) deduction.
As a result, we start seeing sugared types in a lot more test cases,
including some which showed very unfriendly `type-parameter-*-*` types.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Reviewed By: rsmith, #libc, ldionne
Differential Revision: https://reviews.llvm.org/D110216
modernize-loop-convert checks and fixes when a loop that iterates over the
elements of a container can be rewritten from a for(...; ...; ...) style into
the "new" C++11 for-range format. For that, it needs to parse the elements of
that loop, like its init-statement, such as ItType it = cont.begin().
modernize-loop-convert checks whether the loop variable is initialized by a
begin() member function.
When an iterator is initialized with a conversion operator (e.g. for
(const_iterator it = non_const_container.begin(); ...), attempts to retrieve the
name of the initializer expression resulted in an assert, as conversion
operators don't have a valid IdentifierInfo.
I fixed this by making digThroughConstructors dig through conversion operators
as well.
Differential Revision: https://reviews.llvm.org/D113201
Fixes PR#38187. Constructors are actually already checked,
but only as functions, i.e. the check only looks at the
constructor body and not at the initializers, which misses
the (common) case where constructor parameters are moved
as part of an initializer expression.
One remaining false negative is when both the move //and//
the use-after-move occur in constructor initializers.
This is a lot more difficult to handle, though, because
the `bugprone-use-after-move` check is currently based on
a CFG that only takes the body into account, not the
initializers, so e.g. initialization order would have to
manually be considered. I will file a follow-up issue for
this once PR#38187 is closed.
Reviewed By: carlosgalvezp
Differential Revision: https://reviews.llvm.org/D113708
This implements the following changes:
* AutoType retains sugared deduced-as-type.
* Template argument deduction machinery analyses the sugared type all the way
down. It would previously lose the sugar on first recursion.
* Undeduced AutoType will be properly canonicalized, including the constraint
template arguments.
* Remove the decltype node created from the decltype(auto) deduction.
As a result, we start seeing sugared types in a lot more test cases,
including some which showed very unfriendly `type-parameter-*-*` types.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Reviewed By: rsmith
Differential Revision: https://reviews.llvm.org/D110216
This implements the following changes:
* AutoType retains sugared deduced-as-type.
* Template argument deduction machinery analyses the sugared type all the way
down. It would previously lose the sugar on first recursion.
* Undeduced AutoType will be properly canonicalized, including the constraint
template arguments.
* Remove the decltype node created from the decltype(auto) deduction.
As a result, we start seeing sugared types in a lot more test cases,
including some which showed very unfriendly `type-parameter-*-*` types.
Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>
Reviewed By: rsmith
Differential Revision: https://reviews.llvm.org/D110216
Detect when an identifier contains some Right-To-Left characters.
This pass relates to https://trojansource.codes/
Example of misleading source:
short int א = (short int)0;
short int ג = (short int)12345;
int main() {
int א = ג; // a local variable, set to zero?
printf("ג is %d\n", ג);
printf("א is %d\n", א);
}
This is a recommit of 299aa4dfa1 with missing
option registration fixed.
Differential Revision: https://reviews.llvm.org/D112914
Fixes pr40372 (https://bugs.llvm.org/show_bug.cgi?id=40372).
The llvm-header-guard check does not take into account that the path
separator on Windows is `\`, not `/`.
This means that instead of suggesting a header guard in the form of:
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FOO_H
it incorrectly suggests:
C:\LLVM_PROJECT\CLANG_TOOLS_EXTRA\CLANG_TIDY\FOO_H
Differential Revision: https://reviews.llvm.org/D113450
This reverts commit 7f92a1a84b.
It triggers an assert, see http://45.33.8.238/linux/60293/step_9.txt
"AST/Decl.h:277: llvm::StringRef clang::NamedDecl::getName() const: Assertion `Name.isIdentifier() && "Name is not a simple identifier"' failed."
Calling clang-tidy on ClangTidyDiagnosticConsumer.cpp gives a
"unmatched NOLINTBEGIN without a subsequent NOLINTEND" warning.
The "NOLINTBEGIN" and "NOLINTEND" string literals used in the
implementation of `createNolintError()` get mistaken for actual
NOLINTBEGIN/END comments used to suppress clang-tidy warnings.
Rewrite the string literals so that they can no longer be mistaken for
actual suppression comments.
Differential Revision: https://reviews.llvm.org/D113472
Suggests switching the initialization pattern of `absl::Cleanup` instances from the factory function to class template argument deduction (CTAD) in C++17 and higher.
Reviewed By: ymandel
Differential Revision: https://reviews.llvm.org/D113195
The files in compile-commands.json can potentially include duplicates.
Change run-clang-tidy.py so that it does not run on the duplicate entries.
Differential Revision: https://reviews.llvm.org/D112926
The CERT rule ERR33-C can be modeled partially by the existing check
'bugprone-unused-return-value'. The existing check is reused with
a fixed set of checked functions.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D112409
Salman Javed