This is a major change on how we represent nested name qualifications in
the AST.
* The nested name specifier itself and how it's stored is changed. The
prefixes for types are handled within the type hierarchy, which makes
canonicalization for them super cheap, no memory allocation required.
Also translating a type into nested name specifier form becomes a no-op.
An identifier is stored as a DependentNameType. The nested name
specifier gains a lightweight handle class, to be used instead of
passing around pointers, which is similar to what is implemented for
TemplateName. There is still one free bit available, and this handle can
be used within a PointerUnion and PointerIntPair, which should keep
bit-packing aficionados happy.
* The ElaboratedType node is removed, all type nodes in which it could
previously apply to can now store the elaborated keyword and name
qualifier, tail allocating when present.
* TagTypes can now point to the exact declaration found when producing
these, as opposed to the previous situation of there only existing one
TagType per entity. This increases the amount of type sugar retained,
and can have several applications, for example in tracking module
ownership, and other tools which care about source file origins, such as
IWYU. These TagTypes are lazily allocated, in order to limit the
increase in AST size.
This patch offers a great performance benefit.
It greatly improves compilation time for
[stdexec](https://github.com/NVIDIA/stdexec). For one datapoint, for
`test_on2.cpp` in that project, which is the slowest compiling test,
this patch improves `-c` compilation time by about 7.2%, with the
`-fsyntax-only` improvement being at ~12%.
This has great results on compile-time-tracker as well:
This patch also further enables other optimziations in the future, and
will reduce the performance impact of template specialization resugaring
when that lands.
It has some other miscelaneous drive-by fixes.
About the review: Yes the patch is huge, sorry about that. Part of the
reason is that I started by the nested name specifier part, before the
ElaboratedType part, but that had a huge performance downside, as
ElaboratedType is a big performance hog. I didn't have the steam to go
back and change the patch after the fact.
There is also a lot of internal API changes, and it made sense to remove
ElaboratedType in one go, versus removing it from one type at a time, as
that would present much more churn to the users. Also, the nested name
specifier having a different API avoids missing changes related to how
prefixes work now, which could make existing code compile but not work.
How to review: The important changes are all in
`clang/include/clang/AST` and `clang/lib/AST`, with also important
changes in `clang/lib/Sema/TreeTransform.h`.
The rest and bulk of the changes are mostly consequences of the changes
in API.
PS: TagType::getDecl is renamed to `getOriginalDecl` in this patch, just
for easier to rebasing. I plan to rename it back after this lands.
Fixes#136624
Fixes https://github.com/llvm/llvm-project/issues/43179
Fixes https://github.com/llvm/llvm-project/issues/68670
Fixes https://github.com/llvm/llvm-project/issues/92757
The comment for `DiagnosticConsumer::BeginSourceFile()` states that
"diagnostics with source range information are required to only be
emitted in between BeginSourceFile() and EndSourceFile().". While
working on some upcoming changes to the static analyzer, we hit some
crashes when diagnostics were reported from the `EndOfMainFile` callback
in the preprocessor. This turned out to be because
`FrontEndAction::EndSourceFile()` notifies the diagnostic clients of the
end of the source file before it notifies the preprocessor. Thus, the
diagnostics from the preprocessor callback are reported when the
diagnostic client is no longer expecting any diagnostics.
The fix is to swap the order of the `EndSourceFile()` calls so that the
preprocessor is notified first.
I've added asserts to the `ClangTidyDiagnosticConsumer` to catch
unexpected diagnostics outside of a source file. Before swapping the
order of the calls as described above, this causes several failures in
the clang-tidy regression tests. With the swap, there are no failures in
`check-all`.
rdar://141230583
These are identified by misc-include-cleaner. I've filtered out those
that break builds. Also, I'm staying away from llvm-config.h,
config.h, and Compiler.h, which likely cause platform- or
compiler-specific build failures.
This reverts commit e2a885537f11f8d9ced1c80c2c90069ab5adeb1d. Build failures were fixed right away and reverting the original commit without the fixes breaks the build again.
The `DiagnosticOptions` class is currently intrusively
reference-counted, which makes reasoning about its lifetime very
difficult in some cases. For example, `CompilerInvocation` owns the
`DiagnosticOptions` instance (wrapped in `llvm::IntrusiveRefCntPtr`) and
only exposes an accessor returning `DiagnosticOptions &`. One would
think this gives `CompilerInvocation` exclusive ownership of the object,
but that's not the case:
```c++
void shareOwnership(CompilerInvocation &CI) {
llvm::IntrusiveRefCntPtr<DiagnosticOptions> CoOwner = &CI.getDiagnosticOptions();
// ...
}
```
This is a perfectly valid pattern that is being actually used in the
codebase.
I would like to ensure the ownership of `DiagnosticOptions` by
`CompilerInvocation` is guaranteed to be exclusive. This can be
leveraged for a copy-on-write optimization later on. This PR changes
usages of `DiagnosticOptions` across `clang`, `clang-tools-extra` and
`lldb` to not be intrusively reference-counted.
#121323 changed the way the absolute path is computed. Empty file name
will cause absolute path ignore current folder.
This patch add "dummy" file name to avoid this issue
Fixed: #134502
Currently when printing a template argument of expression type, the
expression is converted immediately into a string to be sent to the
diagnostic engine, unsing a fake LangOpts.
This makes the expression printing look incorrect for the current
language, besides being inneficient, as we don't actually need to print
the expression if the diagnostic would be ignored.
This fixes a nastiness with the TemplateArgument constructor for
expressions being implicit, and all current users just passing an
expression to a diagnostic were implicitly going through the template
argument path.
The expressions are also being printed unquoted. This will be fixed in a
subsequent patch, as the test churn is much larger.
PR https://github.com/llvm/llvm-project/pull/91400 broke the usage of
HeaderFilterRegex via config file, because it is now created at a
different point in the execution and leads to a different value.
The result of that is that using HeaderFilterRegex only in the config
file does NOT work, in other words clang-tidy stops triggering warnings
on header files, thereby losing a lot of coverage.
This patch reverts the logic so that the header filter is created upon
calling the getHeaderFilter() function.
Additionally, this patch adds 2 unit tests to prevent regressions in the
future:
- One of them, "simple", tests the most basic use case with a single
top-level .clang-tidy file.
- The second one, "inheritance", demonstrates that the subfolder only
gets warnings from headers within it, and not from parent headers.
Fixes#118009Fixes#121969Fixes#133453
Co-authored-by: Carlos Gálvez <carlos.galvez@zenseact.com>
Resolves: #70930 (and probably latest comments from clangd/clangd#251)
by fixing racing for the shared DiagStorage value which caused messing with args inside the storage and then formatting the following message with getArgSInt(1) == 2:
def err_module_odr_violation_function : Error<
"%q0 has different definitions in different modules; "
"%select{definition in module '%2'|defined here}1 "
"first difference is "
which causes HandleSelectModifier to go beyond the ArgumentLen so the recursive call to FormatDiagnostic was made with DiagStr > DiagEnd that leads to infinite while (DiagStr != DiagEnd).
The Main Idea:
Reuse the existing DiagStorageAllocator logic to make all DiagnosticBuilders having independent states.
Also, encapsulating the rest of state (e.g. ID and Loc) into DiagnosticBuilder.
The last attempt failed -
https://github.com/llvm/llvm-project/pull/108187#issuecomment-2353122096
so was reverted - #108838
Resolves: #70930 (and probably latest comments from
https://github.com/clangd/clangd/issues/251)
by fixing racing for the shared `DiagStorage` value which caused messing
with args inside the storage and then formatting the following message
with `getArgSInt(1)` == 2:
```
def err_module_odr_violation_function : Error<
"%q0 has different definitions in different modules; "
"%select{definition in module '%2'|defined here}1 "
"first difference is "
```
which causes `HandleSelectModifier` to go beyond the `ArgumentLen` so
the recursive call to `FormatDiagnostic` was made with `DiagStr` >
`DiagEnd` that leads to infinite `while (DiagStr != DiagEnd)`.
**The Main Idea:**
Reuse the existing `DiagStorageAllocator` logic to make all
`DiagnosticBuilder`s having independent states.
Also, encapsulating the rest of state (e.g. ID and Loc) into
`DiagnosticBuilder`.
**TODO (if it will be requested by reviewer):**
- [x] add a test (I have no idea how to turn a whole bunch of my
proprietary code which leads `clangd` to OOM into a small public
example.. probably I must try using
[this](https://github.com/llvm/llvm-project/issues/70930#issuecomment-2209872975)
instead)
- [x] [`Diag.CurDiagID !=
diag::fatal_too_many_errors`](https://github.com/llvm/llvm-project/pull/108187#pullrequestreview-2296395489)
- [ ] ? get rid of `DiagStorageAllocator` at all and make
`DiagnosticBuilder` having they own `DiagnosticStorage` coz it seems
pretty small so should fit the stack for short-living
`DiagnosticBuilder` instances
This is a renewed attempt to land @toddlipcon's D34654. The comments on
that patch indicate a broad desire for some ability to ignore headers.
After considering various options, including migrating to std::regex, I
believe this is the best path forward. It's intuitive to have separate
regexes for including headers versus excluding them, and this approach
has the added benefit of being completely opt-in. No existing configs
will break, regardless of existing HeaderFilterRegex values.
This functionality is useful for improving performance when analyzing a
targeted subset of code, as well as in cases where some collection of
headers cannot be modified (third party source, for example).
This patch replaces uses of StringRef::{starts,ends}with with
StringRef::{starts,ends}_with for consistency with
std::{string,string_view}::{starts,ends}_with in C++20.
I'm planning to deprecate and eventually remove
StringRef::{starts,ends}with.
This patch removes the `SourceManager` APIs that create `FileID` from a
`const FileEntry *` in favor of APIs that take `FileEntryRef`. This also
removes a misleading documentation that claims `nullptr` file entry
represents stdin. I don't think that's right, since we just try to
dereference that pointer anyways.
Fix crash when diagnostic is emit with invalid location,
but with attached valid ranges. Diagnostic can contain
invalid location, but SourceManager attached to it still
can be valid, use it in such case or fallback to known
SourceManager.
Fixes: #64602
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D157649
Change default behavior in Clang-tidy and skip by default
module headers parsing. That functionality is buggy and
slow in C++20, and provide tiny benefit.
Reviewed By: carlosgalvezp
Differential Revision: https://reviews.llvm.org/D156161
Since commit 5d12b13b0b26bc58b02ee23c369da8b83240cceb, warnings are
internally classified as errors which skip the check filters.
One use-case is particularly affected: you cannot selectively disable
clang-analyzer-core checks, they are force-enabled because required by others.
So enabling warning as errors will show new (and unwanted) errors !
Co-authored-by: kiwixz <kiwixz@outlook.com>
Fixes: #61969
Reviewed By: carlosgalvezp
Differential Revision: https://reviews.llvm.org/D146520
Re-introduce the patch that was reverted previously.
In the first attempt, the checks would not be able to
read from the global option, since getLocalOrGlobal
only works with string types. Additional logic is needed
in order to support both use cases in the transition
period. All that logic will be removed when the local
options are fully removed.
We have a number of checks designed to analyze problems
in header files only, for example:
bugprone-suspicious-include
google-build-namespaces
llvm-header-guard
misc-definitions-in-header
...
All these checks duplicate the same logic and options
to determine whether a location is placed in the main
source file or in the header. More checks are coming
up with similar requirements.
Thus, to remove duplication, let's move this option
to the top-level configuration of clang-tidy (since
it's something all checks should share).
Add a deprecation notice for all checks that use the
local option, prompting to update to the global option.
Differential Revision: https://reviews.llvm.org/D142655
This patch mechanically replaces None with std::nullopt where the
compiler would warn if None were deprecated. The intent is to reduce
the amount of manual work required in migrating from Optional to
std::optional.
This is part of an effort to migrate from llvm::Optional to
std::optional:
https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716
Before this patch YAML output had default diagnostic level instead of effective level reported to the user on stdout. Wrapper scripts for clang-tidy usually use YAML output and they pick wrong diagnostics level without this patch.
Test Plan: check-clang-tools
Differential Revision: https://reviews.llvm.org/D135367
Adds a flag to `ClangTidyContext` that is used to indicate to checks that fixes will only be applied one at a time.
This is to indicate to checks that each fix emitted should not depend on any other fixes emitted across the translation unit.
I've currently implemented the `IncludeInserter`, `LoopConvertCheck` and `PreferMemberInitializerCheck` to use these support these modes.
Reasoning behind this is in use cases like `clangd` it's only possible to apply one fix at a time.
For include inserter checks, the include is only added once for the first diagnostic that requires it, this will result in subsequent fixes not having the included needed.
A similar issue is seen in the `PreferMemberInitializerCheck` where the `:` will only be added for the first member that needs fixing.
Fixes emitted in `StandaloneDiagsMode` will likely result in malformed code if they are applied all together, conversely fixes currently emitted may result in malformed code if they are applied one at a time.
For this reason invoking `clang-tidy` from the binary will always with `StandaloneDiagsMode` disabled, However using it as a library its possible to select the mode you wish to use, `clangd` always selects `StandaloneDiagsMode`.
This is an example of the current behaviour failing
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E) {
A = D;
B = E; // Fix Here
}
};
```
Incorrectly transformed to:
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E), B(E) {
A = D;
// Fix Here
}
};
```
In `StandaloneDiagsMode`, it gets transformed to:
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E) : B(E) {
A = D;
// Fix Here
}
};
```
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D97121
Support for NOLINT(BEGIN/END) blocks (implemented in D108560) is
currently costly. This patch aims to improve the performance with the
following changes:
- The use of tokenized NOLINTs instead of a series of repetitive ad-hoc
string operations (`find()`, `split()`, `slice()`, regex matching etc).
- The caching of NOLINT(BEGIN/END) block locations. Determining these
locations each time a new diagnostic is raised is wasteful as it
requires reading and parsing the entire source file.
Move NOLINT-specific code from `ClangTidyDiagnosticConsumer` to new
purpose-built class `NoLintDirectiveHandler`.
Differential Revision: https://reviews.llvm.org/D116085
LLVM Programmer’s Manual strongly discourages the use of `std::vector<bool>` and suggests `llvm::BitVector` as a possible replacement.
This patch does just that for clangd and clang-tidy.
Reviewed By: dexonsmith
Differential Revision: https://reviews.llvm.org/D117119
Currently, it's inconsistent that warnings are disabled if they
come from system headers, unless they come from macros.
Typically a user cannot act upon these warnings coming from
system macros, so clang-tidy should ignore them unless the
user specifically requests warnings from system headers
via the corresponding configuration.
This change broke the ProTypeVarargCheck check, because it
was checking for the usage of va_arg indirectly, expanding it
(it's a system macro) to detect the usage of __builtin_va_arg.
The check has been fixed by checking directly what the rule
is about: "do not use va_arg", by adding a PP callback that
checks if any macro with name "va_arg" is expanded. The old
AST matcher is still kept for compatibility with Windows.
Add unit test that ensures warnings from macros are disabled
when not using the -system-headers flag. Document the change
in the Release Notes.
Differential Revision: https://reviews.llvm.org/D116378
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
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
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 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
Re-apply "Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)"
with fixes for the failing ASan tests.
This reverts commit 74add1b6d6d377ab2cdce26699cf798110817e42.
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
Run clang-tidy on all source files under `clang-tools-extra/clang-tidy`
with `-header-filter=clang-tidy.*` and make suggested corrections.
Differential Revision: https://reviews.llvm.org/D112864
To simplify suppressing warnings (for example, for
when multiple check aliases are enabled).
The globbing format reuses the same code as for
globbing when enabling checks, so the semantics
and behavior is identical.
Differential Revision: https://reviews.llvm.org/D111208
This reverts commit c0687e1984a82925918c874b7bb68ad34c32aed0.
There are testing failures being caught by bots.
See http://45.33.8.238/linux/56886/step_8.txt as an example.
