[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/111701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
@@ -0,0 +1,1013 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted -verify
+
+class AClass {
+public:
+ AClass() {}
+ AClass &foo() { return *this; }
+};
+
+void test_bar() {
+ AClass a;
+ // expected-warning@* {{stack nearly exhausted; compilation time may suffer,
and crashes due to stack overflow are likely}}
+ a.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
ilya-biryukov wrote:
It's not that I disagree with DAMP or always prefer DRY, I am just trying to
make the best trade-off in each situation and also feel the right trade-off one
here is closer to DRY.
https://github.com/llvm/llvm-project/pull/111701
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/111701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
@@ -0,0 +1,1013 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted -verify
+
+class AClass {
+public:
+ AClass() {}
+ AClass &foo() { return *this; }
+};
+
+void test_bar() {
+ AClass a;
+ // expected-warning@* {{stack nearly exhausted; compilation time may suffer,
and crashes due to stack overflow are likely}}
+ a.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
ilya-biryukov wrote:
I can understand the desire to keep important information visible, but I find
the fact that humans have a limited ability to comprehend large chunks of
information important.
For a test like this, the important information for me is:
- how deep is the expression,
- what kind of an expression it is.
The vast size of the file makes it harder, not simpler, to get that information
and it also requires extra actions (going to the end of the file) to make sure
there isn't something else that the reader needs to know.
Preprocessor is generally very reliable and well-understood, so I'd still lean
towards the smaller version here.
https://github.com/llvm/llvm-project/pull/111701
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
ilya-biryukov wrote: The comments on this thread bugged me, so I managed to find a way to achieve the same effect without adding any new flags. #112015 should give the deduplication that I want to get here without introducing any new flags, through a tweak to the mechanism of non-affecting source locations. I will close this PR, let's continue discussion in #112015. https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
https://github.com/ilya-biryukov closed https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [ASTWriter] Detect more non-affecting FileIDs to reduce source location duplication (PR #112015)
ilya-biryukov wrote: This is a change that should remove the need for a hack from #88893, reaching the same effect while keeping the command-line interface as it is today. https://github.com/llvm/llvm-project/pull/112015 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [ASTWriter] Detect more non-affecting FileIDs to reduce source location duplication (PR #112015)
https://github.com/ilya-biryukov created https://github.com/llvm/llvm-project/pull/112015 Currently, any FileID that references a module map file that was required for a compilation is considered as affecting. This misses an important opportunity to reduce the source location space taken by the resulting PCM. In particular, consider the situation where the same module map file is passed multiple times in the dependency chain: ```shell $ clang -fmodule-map-file=foo.modulemap ... -o mod1.pcm $ clang -fmodule-map-file=foo.modulemap -fmodule-file=mod1.pcm ... -o mod2.pcm ... $ clang -fmodule-map-file=foo.modulemap -fmodule-file=mod$((N-1)).pcm ... -o mod$N.pcm ``` Because `foo.modulemap` is read before reading any of the `.pcm` files, we have to create a unique `FileID` for it when creating each module. However, when reading the `.pcm` files, we will reuse the `FileID` loaded from it for the same module map file and the `FileID` we created can never be used again, but we will still not mark it as affecting and it will take the source location space in the output PCM. For a chain of N dependencies, this results in the file taking `N * (size of file)` source location space, which could be significant. For examples, we observer internally that some targets that run out of 2GB of source location space end up wasting up to 20% of that space in module maps as described above. I take extra care to still write the InputFile entries for those files, which has not been done before. It is required for ClangScanDeps to properly function. The change in the output of one of the ClangScanDeps tests is attributed to that: we now add a bit more module maps to the output in some tricky cases. E.g. in the test case the module map file is required and is referenced by another top-level module map, adding it is redundant, but should not be breaking. >From b6508a13d1a115bb3acb54590833674f4158814c Mon Sep 17 00:00:00 2001 From: Ilya Biryukov Date: Thu, 19 Sep 2024 20:18:36 +0200 Subject: [PATCH] [ASTWriter] Detect more non-affecting FileIDs to reduce duplication of source locations Currently, any FileID that references a module map file that was required for a compilation is considered as affecting. This misses an important opportunity to reduce the source location space taken by the resulting PCM. In particular, consider the situation where the same module map file is passed multiple times in the dependency chain: ```shell $ clang -fmodule-map-file=foo.modulemap ... -o mod1.pcm $ clang -fmodule-map-file=foo.modulemap -fmodule-file=mod1.pcm ... -o mod2.pcm ... $ clang -fmodule-map-file=foo.modulemap -fmodule-file=mod$((N-1)).pcm ... -o mod$N.pcm ``` Because `foo.modulemap` is read before reading any of the `.pcm` files, we have to create a unique `FileID` for it when creating each module. However, when reading the `.pcm` files, we will reuse the `FileID` loaded from it for the same module map file and the `FileID` we created can never be used again, but we will still not mark it as affecting and it will take the source location space in the output PCM. For a chain of N dependencies, this results in the file taking `N * (size of file)` source location space, which could be significant. For examples, we observer internally that some targets that run out of 2GB of source location space end up wasting up to 20% of that space in module maps as described above. I take extra care to still write the InputFile entries for those files, which has not been done before. It is required for ClangScanDeps to properly function. The change in the output of one of the ClangScanDeps tests is attributed to that: we now add a bit more module maps to the output in some tricky cases. E.g. in the test case the module map file is required and is referenced by another top-level module map, adding it is redundant, but should not be breaking. --- clang/include/clang/Serialization/ASTWriter.h | 3 + clang/lib/Serialization/ASTReader.cpp | 6 ++ clang/lib/Serialization/ASTWriter.cpp | 41 ++--- .../ClangScanDeps/modules-extern-submodule.c | 2 +- ...rune-non-affecting-module-map-repeated.cpp | 85 +++ 5 files changed, 126 insertions(+), 11 deletions(-) create mode 100644 clang/test/Modules/prune-non-affecting-module-map-repeated.cpp diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h index 10a50b711043a8..6b03300d59adda 100644 --- a/clang/include/clang/Serialization/ASTWriter.h +++ b/clang/include/clang/Serialization/ASTWriter.h @@ -491,6 +491,9 @@ class ASTWriter : public ASTDeserializationListener, /// Mapping from a source location entry to whether it is affecting or not. llvm::BitVector IsSLocAffecting; + /// Mapping from a source location entry to whether it must be included as + /// input file. + llvm::BitVector IsSLocFileEntryAffecting; /// Mapping from \c FileID to an index into the FileID adjustment tabl
[clang] [C++20][Modules] Fix crash when function and lambda inside loaded from different modules (PR #109167)
ilya-biryukov wrote: > > I've checked #111992 and it does fix the problem, so let's land it? > > Yes, I would like to create a test case to don't regress this feature in > future. I need to reduce libc++ `functional` header to something smaller. Right, definitely +1 to adding a test case. I decided to hold off that comment until the PR moves away from the draft state, because I thought that's something that's on your mind anyway. https://github.com/llvm/llvm-project/pull/109167 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Load function body from the module that gives canonical decl (PR #111992)
@@ -10057,15 +10057,18 @@ void ASTReader::finishPendingActions() {
// For a function defined inline within a class template, force the
// canonical definition to be the one inside the canonical definition of
// the template. This ensures that we instantiate from a correct view
- // of the template.
+ // of the template. This behaviour seems to be important only for inline
+ // friend functions. For normal member functions, it might results in
+ // selecting canonical decl from module A but body from module B.
//
// Sadly we can't do this more generally: we can't be sure that all
// copies of an arbitrary class definition will have the same members
// defined (eg, some member functions may not be instantiated, and some
// special members may or may not have been implicitly defined).
- if (auto *RD = dyn_cast(FD->getLexicalParent()))
-if (RD->isDependentContext() && !RD->isThisDeclarationADefinition())
- continue;
+ if (FD->getFriendObjectKind())
ilya-biryukov wrote:
Does it mean we can still potentially have this problem for `friend` functions?
Or are there some invariants in Clang/C++ itself that stop that from happening?
If we have a small example, it'd be interesting to check that adding `friend`
does not cause the problem to resurface.
https://github.com/llvm/llvm-project/pull/111992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [C++20][Modules] Fix crash when function and lambda inside loaded from different modules (PR #109167)
ilya-biryukov wrote: > > Here is the reproducer that crashes at head. > > [lambdas.tgz](https://github.com/user-attachments/files/17329901/lambdas.tgz) > > I reproduced this issue on my machine and confirm that it is indeed due to my > changes. #111992 has fix that seems to fix the reproducer and doesn't > introduce new issue as far as I know. @ilya-biryukov could you please try it > on your full test case? I've checked #111992 and it does fix the problem, so let's land it? https://github.com/llvm/llvm-project/pull/109167 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
ilya-biryukov wrote: The breakage from buildkite is realted: we don't seem to run out of stack space on Windows, unfortunately it's quite common for these things to be vary between platforms. We could either limit the tests to Linux, or increase the depth, or figure some other way to reconcile it 🤷 https://github.com/llvm/llvm-project/pull/111701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
@@ -0,0 +1,1013 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -Wstack-exhausted -verify
+
+class AClass {
+public:
+ AClass() {}
+ AClass &foo() { return *this; }
+};
+
+void test_bar() {
+ AClass a;
+ // expected-warning@* {{stack nearly exhausted; compilation time may suffer,
and crashes due to stack overflow are likely}}
+ a.foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
ilya-biryukov wrote:
Could we use preprocessor to reduce the size of the file?
The typical trick I use is:
```cpp
#define CALLS1 .foo().foo().foo().foo().foo().foo().foo().foo().foo().foo()
#define CALLS2 CALLS1().CALLS1().CALLS1().CALLS1(). ...
...
#define CALLS${N} CALLS${N-1}
a.CALLS();
```
https://github.com/llvm/llvm-project/pull/111701
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
@@ -5817,7 +5817,10 @@ LValue CodeGenFunction::EmitHLSLArrayAssignLValue(const
BinaryOperator *E) {
LValue CodeGenFunction::EmitCallExprLValue(const CallExpr *E,
llvm::CallBase **CallOrInvoke) {
- RValue RV = EmitCallExpr(E, ReturnValueSlot(), CallOrInvoke);
ilya-biryukov wrote:
Would it be better to wrap a more generic function? I'm thinking about
`EmitLValue`
```cpp
LValue CodeGenFunction::EmitLValue(const Expr *E) {
LValue R;
R = runWithSufficientStackSpace([] { R = EmitLValueImpl(E); }, ...);
return R;
}
// The current function...
LValue CodeGenFunction::EmitLValueImpl(const Expr *E) {
ApplyDebugLocation DL(*this, E);
switch (E->getStmtClass()) {
...
}
```
That way we can cover a broader class of recursive expressions. I'm not sure
the coverage is complete,
but should probably be quite good for a start.
(We could also add a few tests for deep expressions that don't involve calls).
https://github.com/llvm/llvm-project/pull/111701
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
https://github.com/ilya-biryukov commented: I have a few suggestions, but definitely support this. I've also added a few folks suggested by GitHub as reviewers to make sure we have coverage of someone who looks at CodeGen. https://github.com/llvm/llvm-project/pull/111701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Fix segmentation fault caused by stack overflow on deeply nested expressions (PR #111701)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/111701 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][LLVM Demangler] Add an assertion that validates that all mang… (PR #111391)
ilya-biryukov wrote: > I agree completely. I have a preference for the second thing as well. We can > have this be a warning-as-default-error type thing, which allows us to > disable this with a flag, but is still an error. I MIGHT suggest using the > functionality to see if that diagnostic is on before doing the test (due to > how much additional work there is here), but IMO, that is a much better way > forward. đź‘Ť I guess that's a good signal that this is the way to go. AFAIK, @VitaNuo has already started exploring this direction. Wrt to enabling this warning, I think we should probably start by having this warning disabled because there seems to be too many failures, but we could gradually roll it out: - start with a warning that's disabled, - collect issues notable in LLVM tests (there's plenty AFAIU) and also from real projects (we can run with the warning internally over Google's codebase to get a representative sample), - fix all demangling issues present in the tests and the most common ones from other projects, - enabling the flag by default. @erichkeane how do you feel about having it enabled given that this does not really block users, but merely points at demangling issues in LLVM's demangler (that they don't necessarily even use). It feels like this should be internal, but then we won't get any bug reports. I feel like we should at least fix the majority of the issues before enabling it (hence the rough roadmap above). https://github.com/llvm/llvm-project/pull/111391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Fix out-of-bounds access to std::unique_ptr (PR #111581)
https://github.com/ilya-biryukov approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/111581 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [clang][LLVM Demangler] Add an assertion that validates that all mang… (PR #111391)
ilya-biryukov wrote: Sorry for jumping in late. I think Erich is on point that having a flag that controls an assertion is a bit of a red flag as we are mixing build configuration and runtime configuration. It is at least unusual and may cause confusion. After thinking about this a bit more, should we maybe go all-in on one of the approaches? - either use build configuration: add a new build flag that controls this assertion. Only assert when assertions are enabled and the new build flag is defined. - or use runtime configure always, e.g. add diagnostics for names that can't be demangled. It should make finding those issues much easier (one can run the compiler to produce a list of names that can't be demangled with locations pointing at code that helps to identify where those names come from). I would probably vouch for the second approach. The only downside that I see is that we have to explore the warning flag to users (right?) and this is something that should be internal (-cc1). What do people think of that proposal? https://github.com/llvm/llvm-project/pull/111391 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add isTrivial() and isTriviallyCopyable() AST matchers (PR #90634)
@@ -5444,6 +5444,43 @@ AST_MATCHER(FunctionDecl, isDefaulted) {
return Node.isDefaulted();
}
+/// Matches trivial methods and types.
ilya-biryukov wrote:
I do not think is actually matches `types`, but rather `classes` or
`CXXRecordDecl`.
And the question is: should it?
I believe we could extend this matcher to also work on types and type locs.
Do you see any downsides to that? (same for the other matcher)
https://github.com/llvm/llvm-project/pull/90634
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add isTrivial() and isTriviallyCopyable() AST matchers (PR #90634)
https://github.com/ilya-biryukov requested changes to this pull request. https://github.com/llvm/llvm-project/pull/90634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add isTrivial() and isTriviallyCopyable() AST matchers (PR #90634)
ilya-biryukov wrote: I remember back in the day that adding new matchers was a problem because it grew the size of the files beyond some supported compiler's limit. Could @AaronBallman or someone else knowledgable about this(cc @kadircet) take a look and confirm adding matchers is fine now? https://github.com/llvm/llvm-project/pull/90634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1682,7 +1690,7 @@ class DeclContext {
/// True if a valid hash is stored in ODRHash. This should shave off some
/// extra storage and prevent CXXRecordDecl to store unused bits.
-uint64_t ODRHash : 26;
+uint64_t ODRHash : 25;
ilya-biryukov wrote:
Oh, wow, so we have to chip off the bits from an ODR hash when we add a new bit
to class state?
This is unfortunate, especially given that it is somewhat hard to measure which
trade-offs this entails? (How many bits of ODR hash is too little?)
I don't think it is a reason to block this PR, but it would be nice to come up
with a way to measure which number of bits we cannot go under.
A major comment: you also have to update the place that sets the ODRHash. For
some reason, we prefer to have the top 26 bits, not the low 26 bits, see:
https://github.com/llvm/llvm-project/blob/fb6feb86a7dc324dcb2516397ba3fc5fe05ea584/clang/lib/AST/Decl.cpp#L5219
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
ilya-biryukov wrote: @AaronBallman this seems to have progressed much further after your review and is currently pending on input from you. Could you please do a round of review here/in the RFC? (Or ask someone else from the community to address your concerns and be a reviewer instead, not sure if that's more useful) @cor3ntin and also the same question. Could you take another look to see if your concerns were addressed? https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][frontend] Add support for attribute plugins for statement attributes (PR #110334)
ilya-biryukov wrote: This looks like a relatively straightforward extension in line with the current Clang interfaces. Therefore, I feel we should accept this. However, please wait for feedback from Clang owners to make sure this does not go against some non-obvious trade-offs or future plans that Clang has. cc @AaronBallman, @cor3ntin https://github.com/llvm/llvm-project/pull/110334 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builtin_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
ilya-biryukov wrote: I have also left a few comments in the parallel discussion on the RFC. https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builtin_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
@@ -3158,6 +3161,33 @@ checkBuiltinTemplateIdType(Sema &SemaRef,
BuiltinTemplateDecl *BTD,
int64_t N = Index.getExtValue();
return Ts.getPackAsArray()[N].getAsType();
}
+ case BTK__type_pack_dedup: {
+assert(Converted.size() == 2 && "__builtin_type_pack_dedup should be given
"
+"a template and a parameter pack");
+TemplateArgument Template = Converted[0];
+TemplateArgument Ts = Converted[1];
+if (Template.isDependent() || Ts.isDependent())
ilya-biryukov wrote:
We could remove the dependency on the template, but I don't think we can ignore
the dependency inside the types themselves.
Consider
```cpp
template class TL = TypeList>
struct Foo {
// 1. the substitution has to be postponed until T and U are known.
using result = __dedup;
// 2. this is already computed right away.
using result2 = __dedup;
// 3. this **could** be computed right away, but it is not now.
using result3 = __dedup
};
static_assert(__is_same(TypeList, Foo::result));
static_assert(__is_same(TypeList, Foo::result));
```
- Is there something I'm missing that would allow (1) to be substituted earlier?
- Do you feel strongly that we need to do (3) ?
> Be aware that holding this specialization will make this builtin appear in
> mangling, so I'd avoid doing that unless necessary.
I do not think we can avoid this completely (see above) and it's not
unprecedented: other builtin templates can already appear in mangling too when
the arguments are dependent. So I do think it's necessary and I'm not sure
which problems it carries.
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builtin_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
ilya-biryukov wrote: Friendly ping for another round of review. https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builtin_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builin_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __builint_type_pack_dedup template to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Propagate lifetimebound from formal parameters to those in the canonical declaration and use that for analysis (PR #107627)
@@ -266,6 +262,20 @@ void Sema::inferLifetimeBoundAttribute(FunctionDecl *FD) {
LifetimeBoundAttr::CreateImplicit(Context, FD->getLocation()));
}
}
+ } else if (auto *CanonDecl = FD->getCanonicalDecl(); FD != CanonDecl) {
+// Propagate the lifetimebound attribute from parameters to the canonical
+// declaration.
+// Note that this doesn't include the implicit 'this' parameter, as the
+// attribute is applied to the function type in that case.
+const unsigned int NP = std::min(NumParams, CanonDecl->getNumParams());
+for (unsigned int I = 0; I < NP; ++I) {
+ auto *CanonParam = CanonDecl->getParamDecl(I);
+ if (!CanonParam->hasAttr() &&
+ FD->getParamDecl(I)->hasAttr()) {
ilya-biryukov wrote:
I'll probably have more comments, but just wanted to point out that a much more
appropriate place for this logic would be `Sema::mergeDeclAttributes` and the
`mergeParamDeclAttributes` helper function, in particular.
https://github.com/llvm/llvm-project/pull/107627
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Propagate lifetimebound from formal parameters to those in the canonical declaration and use that for analysis (PR #107627)
@@ -462,14 +462,16 @@ static void visitFunctionCallArguments(IndirectLocalPath
&Path, Expr *Call,
}
}
- for (unsigned I = 0,
-N = std::min(Callee->getNumParams(), Args.size());
- I != N; ++I) {
-if (CheckCoroCall || Callee->getParamDecl(I)->hasAttr())
- VisitLifetimeBoundArg(Callee->getParamDecl(I), Args[I]);
+ const FunctionDecl *CanonCallee = Callee->getCanonicalDecl();
+ const unsigned int NP =
ilya-biryukov wrote:
NIT: LLVM uses `unsigned`, could you remove `int` to be consistent with the
rest of the code?
Also, using `const` for local variables is not very common. I'm not sure if the
LLVM Style Guide has a position on it, but it's at least inconsistent with the
code around the change, so we should probably lean on the side of **not**
adding const.
https://github.com/llvm/llvm-project/pull/107627
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -3158,6 +3161,33 @@ checkBuiltinTemplateIdType(Sema &SemaRef,
BuiltinTemplateDecl *BTD,
int64_t N = Index.getExtValue();
return Ts.getPackAsArray()[N].getAsType();
}
+ case BTK__type_pack_dedup: {
+assert(Converted.size() == 2 && "__builtin_type_pack_dedup should be given
"
+"a template and a parameter pack");
+TemplateArgument Template = Converted[0];
+TemplateArgument Ts = Converted[1];
+if (Template.isDependent() || Ts.isDependent())
+ return Context.getCanonicalTemplateSpecializationType(TemplateName(BTD),
+Converted);
+
+assert(Template.getKind() == clang::TemplateArgument::Template);
+assert(Ts.getKind() == clang::TemplateArgument::Pack);
+TemplateArgumentListInfo SyntheticTemplateArgs;
+llvm::SmallDenseSet Seen;
+// Synthesize a new template argument list, removing duplicates.
+for (auto T : Ts.getPackAsArray()) {
+ assert(T.getKind() == clang::TemplateArgument::Type);
+ if (!Seen.insert(T.getAsType().getCanonicalType()).second)
+continue;
+ SyntheticTemplateArgs.addArgument(TemplateArgumentLoc(
+ TemplateArgument(T), SemaRef.Context.getTrivialTypeSourceInfo(
+ T.getAsType(),
+ /*FIXME: add location*/ SourceLocation(;
ilya-biryukov wrote:
After looking at the code, I am afraid the not-very-precise source location
will stay a limitation of this builtin (it also seems like it's the case for
some other cases involving packs? as soon as a template argument gets added
into a pack, it seems we should be loosing its location information at least in
some cases)
We do not seem to be tracking the `TemplateArgumentLoc`s of `TempateArgument`s
we get from the pack very precisely. While it's possible to poke at the
`TemplateArgumentList` provided into the function and match them with the types
I get from `Converted`, I am not entirely sure what the structure of the
`TemplateArgumentListInfo` is [1] and whether the complexity around it is worth
it?
@zygoloid any thoughts on this? is it worth digging into the various
representations the passed `TemplateArgumentListInfo` might have or is it a
dead end?
[1]: is it always flattened? does it always have packs? can it be both?
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
ilya-biryukov wrote: > I don't think the rfc has reached its conclusion yet, and consensus has not > been called (for example, i still need to think about whether my questions > were addressed) so we should wait for the RFC process before continuing with > that PR. > > Thanks Thanks for explicitly calling this out. There were no replies from you on the RFC for some time, so it was unclear whether there is anything left. We will be waiting for your feedback on Discourse. https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -3158,6 +3161,33 @@ checkBuiltinTemplateIdType(Sema &SemaRef,
BuiltinTemplateDecl *BTD,
int64_t N = Index.getExtValue();
return Ts.getPackAsArray()[N].getAsType();
}
+ case BTK__type_list_dedup: {
+assert(Converted.size() == 2 &&
+ "__type_list_dedup should be given a template and a parameter
pack");
+TemplateArgument Template = Converted[0];
+TemplateArgument Ts = Converted[1];
+if (Template.isDependent() || Ts.isDependent())
+ return Context.getCanonicalTemplateSpecializationType(TemplateName(BTD),
+Converted);
+
+assert(Template.getKind() == clang::TemplateArgument::Template);
+assert(Ts.getKind() == clang::TemplateArgument::Pack);
+TemplateArgumentListInfo SyntheticTemplateArgs;
+llvm::DenseSet Seen;
ilya-biryukov wrote:
Done.
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -0,0 +1,57 @@ +// RUN: %clang_cc1 %s -verify ilya-biryukov wrote: Done. I have also added a few tests with arrays and pointers, which should also exhibit a few more combinations of types that subsume qualifiers and could be different when canonical/non-canonical. https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -309,7 +309,10 @@ enum BuiltinTemplateKind : int {
BTK__make_integer_seq,
/// This names the __type_pack_element BuiltinTemplateDecl.
- BTK__type_pack_element
+ BTK__type_pack_element,
+
+ /// This names the __type_list_dedup BuiltinTemplateDecl.
ilya-biryukov wrote:
Thanks, I don't actually know why I didn't pick `type_pack` prefix in the first
place. I think I was confused when I started and thought that
`type_pack_element` was more different than it ended up being.
It's now `__builtin_type_pack_dedup`, as suggested.
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [llvm] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -309,7 +309,10 @@ enum BuiltinTemplateKind : int {
BTK__make_integer_seq,
/// This names the __type_pack_element BuiltinTemplateDecl.
- BTK__type_pack_element
+ BTK__type_pack_element,
+
+ /// This names the __type_list_dedup BuiltinTemplateDecl.
+ BTK__type_list_dedup,
ilya-biryukov wrote:
> Unless we think that builtin templates were a mistake, and we instead would
> prefer to deprecate and eventually remove our support for builtin templates?
I certainly feel they are a useful tool to have and we should probably keep
them.
I'm definitely very supportive of reducing the boilerplate.
On that front, @philnik777 do you feel like you digging out that local patch or
should I take a stab at it?
In any case, I'd do this in a separate change. The boilerplate present in this
commit is still very much manageable (and already done), so I don't think that
blocking this PR on removing this boileerplate would be my first choice.
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
ilya-biryukov wrote: @AaronBallman @cor3ntin I believe we are getting close to finalizing this PR. Would you be okay with this feature landing and myself approving this when it's ready? There was some discussion here and in the RFC, but I don't think there was explicit approval (or objection) to land this, so I wanted to clarify this. https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -743,6 +743,12 @@ void InitListChecker::FillInEmptyInitForField(unsigned
Init, FieldDecl *Field,
ILE->updateInit(SemaRef.Context, Init, Filler);
return;
}
+
+if (Field->hasAttr()) {
+ SemaRef.Diag(ILE->getExprLoc(), diag::warn_field_requires_explicit_init)
ilya-biryukov wrote:
Your other comment seems right.
`InitListChecker` should not produce any diagnostics when `VerifyOnly == true`,
so we should only report the warning when the flag is false.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1472,3 +1472,49 @@ template struct Outer {
};
};
Outer::Inner outerinner;
+
+void aggregate() {
+ struct S {
+[[clang::requires_explicit_initialization]] int x;
+int y;
+int z = 12;
+[[clang::requires_explicit_initialization]] int q = 100;
+static void foo(S) { }
+ };
+
+ struct D : S { // expected-warning {{not explicitly initialized}}
+int f1;
+int f2 [[clang::requires_explicit_initialization]];
+ };
+
+ struct C {
+[[clang::requires_explicit_initialization]] int w;
+C() = default; // Test pre-C++20 aggregates
+ };
+
+ S::foo(S{1, 2, 3, 4});
+ S::foo(S{.x = 100, .q = 100});
+ S::foo(S{.x = 100}); // expected-warning {{'q' is not explicitly
initialized}} expected-warning {{'q' is not explicitly initialized}}
+ S s{.x = 100, .q = 100};
+ (void)s;
+ S t{.q = 100}; // expected-warning {{'x' is not explicitly initialized}}
expected-warning {{'x' is not explicitly initialized}}
+ (void)t;
+ S *ptr1 = new S; // expected-warning {{not explicitly initialized}}
+ delete ptr1;
+ S *ptr2 = new S{.x = 100, .q = 100};
+ delete ptr2;
+#if __cplusplus >= 202002L
+ D a({}, 0); // expected-warning {{'x' is not explicitly initialized}}
expected-warning {{'f2' is not explicitly initialized}}
+ (void)a;
+#else
+ C a; // expected-warning {{not explicitly initialized}}
+ (void)a;
+#endif
+ D b{.f2 = 1}; // expected-warning {{'x' is not explicitly initialized}}
expected-warning {{'q' is not explicitly initialized}}
ilya-biryukov wrote:
As an idea for future improvements: we could also collect all unitialized
fields and emit a single diagnostic that lists them all (with notes to the
locations of the fields).
However, I think this is good enough for the first version, I don't necessarily
feel we should do it right away.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -2148,6 +2158,19 @@ void
CXXRecordDecl::completeDefinition(CXXFinalOverriderMap *FinalOverriders) {
for (conversion_iterator I = conversion_begin(), E = conversion_end();
I != E; ++I)
I.setAccess((*I)->getAccess());
+
+ ASTContext &Context = getASTContext();
+ if (!Context.getLangOpts().CPlusPlus20 && hasUserDeclaredConstructor()) {
ilya-biryukov wrote:
We should probably complaint only if the type is aggregate in the first place,
right?
See https://gcc.godbolt.org/z/sa94xa9or for an example code.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -743,6 +743,12 @@ void InitListChecker::FillInEmptyInitForField(unsigned
Init, FieldDecl *Field,
ILE->updateInit(SemaRef.Context, Init, Filler);
return;
}
+
+if (Field->hasAttr()) {
+ SemaRef.Diag(ILE->getExprLoc(), diag::warn_field_requires_explicit_init)
+ << Field;
ilya-biryukov wrote:
Maybe we could also add the `note_entity_declared_at` pointing at field's
location?
It will mostly be redundant, but can sometimes really come in handy if the
field names happen to clash with base classes, e.g.
https://gcc.godbolt.org/z/Pzx9PE3Mh
However, this also adds noise.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -119,6 +119,14 @@ FIELD(HasInitMethod, 1, NO_MERGE) /// within anonymous unions or structs. FIELD(HasInClassInitializer, 1, NO_MERGE) +/// Custom attribute that is True if any field is marked as explicit in a type ilya-biryukov wrote: Suggestion: could we mention the attribute name? E.g. "marked as requiring init with [[]]" https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov commented: See my comment about `VerifyOnly` and duplicate diagnostics. The rest are small NITs. https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/108197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
https://github.com/ilya-biryukov approved this pull request. LGTM from my side if all other tests pass. Given that it's a small change and bugfix, I think it should be okay to land this without waiting for other reviewers. We could always follow up with more fixes if they have additional comments. https://github.com/llvm/llvm-project/pull/108197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
@@ -113,9 +113,11 @@ class TreeTransform {
class ForgetPartiallySubstitutedPackRAII {
Derived &Self;
TemplateArgument Old;
+Sema::ArgumentPackSubstitutionIndexRAII ResetPackSubstIndex;
ilya-biryukov wrote:
Could you add a short comment mentioning that we need this because many code
paths assume correct index corresponds to the pack being present and do not do
any extra checking?
https://github.com/llvm/llvm-project/pull/108197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/108197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
ilya-biryukov wrote:
I have spent some time poking at the code and looking at the debugger and came
up with a smaller repro, see https://gcc.godbolt.org/z/6ccPPd6hz:
```cpp
int bar(...);
template struct Int {};
template
constexpr auto foo(T... x) -> decltype(bar(T(x)...)) { return 1; }
template
constexpr auto baz(Int(T())>... x) -> int { return 1; }
static_assert(baz, Int<2>, Int<3>>(Int<1>(), Int<2>(), Int<3>(),
Int<4>()) == 1);
```
I hope this helps.
https://github.com/llvm/llvm-project/pull/108197
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
ilya-biryukov wrote: Other than the test, the idea of the change does seem correct, here's the relevant comment from `Sema.h`: ```cpp /// The current index into pack expansion arguments that will be /// used for substitution of parameter packs. /// /// The pack expansion index will be -1 to indicate that parameter packs /// should be instantiated as themselves. Otherwise, the index specifies /// which argument within the parameter pack will be used for substitution. int ArgumentPackSubstitutionIndex; ``` It looks clear that when `RetainExpansion == true`, we would like the behavior that `ArgumentPackSubstitutionIndex == -1` produces. And given that we already forget the particular partially substituted pack, it's no surprise that using `ArgumentPackSubstitutionIndex` gives us an out-of-bounds access. Maybe there are certain intricacies I am not getting, but I believe your direction to put this into `ForgetPartiallySubstitutedPackRAII` is the right way forward. cc @zygoloid @shafik @cor3ntin in case they want to chime in. https://github.com/llvm/llvm-project/pull/108197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Do not substitute parameter pack while retaining the pack expansion (PR #108197)
https://github.com/ilya-biryukov commented: Sorry for not looking closer yet, but could we get a test case? It would make it much easier to review this change. https://github.com/llvm/llvm-project/pull/108197 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -38,9 +40,11 @@ #include "clang/Sema/Template.h" #include "clang/Sema/TemplateDeduction.h" #include "llvm/ADT/BitVector.h" +#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/SmallBitVector.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" +#include "llvm/Support/raw_ostream.h" ilya-biryukov wrote: thanks! removed it. https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/106730
>From a46885df62ff64f355abb010f778d84309acd10f Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 23 Aug 2024 17:27:26 +0200
Subject: [PATCH 1/5] [Clang] Add __type_list_dedup builtin to deduplicate
types in template arguments
This allows to deduplicate the type lists efficiently in C++. It is
possible to achieve the same effect via template metaprogramming, but
performance of the resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase
and this builtin allows to have significant savings (up to 25% of
compile time on targets that already take 3 minutes to compile).
The same builtin is also used widely enough in the codebase that we
expect a savings from a long tail of uses, too, although it is hard to
put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the
exsisting machinery for builtin templates.
---
.../clangd/unittests/FindTargetTests.cpp | 6
clang/include/clang/AST/ASTContext.h | 11 ++
clang/include/clang/AST/DeclID.h | 3 ++
clang/include/clang/Basic/Builtins.h | 5 ++-
clang/lib/AST/ASTContext.cpp | 8 +
clang/lib/AST/ASTImporter.cpp | 3 ++
clang/lib/AST/DeclTemplate.cpp| 31
clang/lib/Lex/PPMacroExpansion.cpp| 1 +
clang/lib/Sema/SemaLookup.cpp | 7 +++-
clang/lib/Sema/SemaTemplate.cpp | 36 +--
clang/lib/Serialization/ASTReader.cpp | 6
clang/lib/Serialization/ASTWriter.cpp | 3 ++
.../test/Import/builtin-template/Inputs/S.cpp | 7
clang/test/Import/builtin-template/test.cpp | 10 +-
clang/test/PCH/type_list_dedup.cpp| 20 +++
.../SemaTemplate/temp-param-list-dedup.cpp| 33 +
16 files changed, 185 insertions(+), 5 deletions(-)
create mode 100644 clang/test/PCH/type_list_dedup.cpp
create mode 100644 clang/test/SemaTemplate/temp-param-list-dedup.cpp
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 3220a5a6a98250..d9f788cbf2a8a2 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -731,6 +731,12 @@ TEST_F(TargetDeclTest, BuiltinTemplates) {
using type_pack_element = [[__type_pack_element]];
)cpp";
EXPECT_DECLS("TemplateSpecializationTypeLoc", );
+
+ Code = R"cpp(
+template Templ, class... Types>
+using type_list_dedup = [[__type_list_dedup]];
+ )cpp";
+ EXPECT_DECLS("TemplateSpecializationTypeLoc", );
}
TEST_F(TargetDeclTest, MemberOfTemplate) {
diff --git a/clang/include/clang/AST/ASTContext.h
b/clang/include/clang/AST/ASTContext.h
index 89bb5768dbd40d..1960ec1e99f652 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -401,6 +401,9 @@ class ASTContext : public RefCountedBase {
/// The identifier '__type_pack_element'.
mutable IdentifierInfo *TypePackElementName = nullptr;
+ /// The identifier '__type_list_dedup'.
+ mutable IdentifierInfo *TypeListDedupName = nullptr;
+
QualType ObjCConstantStringType;
mutable RecordDecl *CFConstantStringTagDecl = nullptr;
mutable TypedefDecl *CFConstantStringTypeDecl = nullptr;
@@ -608,6 +611,7 @@ class ASTContext : public RefCountedBase {
mutable ExternCContextDecl *ExternCContext = nullptr;
mutable BuiltinTemplateDecl *MakeIntegerSeqDecl = nullptr;
mutable BuiltinTemplateDecl *TypePackElementDecl = nullptr;
+ mutable BuiltinTemplateDecl *TypeListDedupDecl = nullptr;
/// The associated SourceManager object.
SourceManager &SourceMgr;
@@ -1115,6 +1119,7 @@ class ASTContext : public RefCountedBase {
ExternCContextDecl *getExternCContextDecl() const;
BuiltinTemplateDecl *getMakeIntegerSeqDecl() const;
BuiltinTemplateDecl *getTypePackElementDecl() const;
+ BuiltinTemplateDecl *getTypeListDedupDecl() const;
// Builtin Types.
CanQualType VoidTy;
@@ -2006,6 +2011,12 @@ class ASTContext : public RefCountedBase {
return TypePackElementName;
}
+ IdentifierInfo *getTypeListDedupName() const {
+if (!TypeListDedupName)
+ TypeListDedupName = &Idents.get("__type_list_dedup");
+return TypeListDedupName;
+ }
+
/// Retrieve the Objective-C "instancetype" type, if already known;
/// otherwise, returns a NULL type;
QualType getObjCInstanceType() {
diff --git a/clang/include/clang/AST/DeclID.h b/clang/include/clang/AST/DeclID.h
index 81454a247229f5..41cecf1b8a9ebf 100644
--- a/clang/include/clang/AST/DeclID.h
+++ b/clang/include/clang/AST/DeclID.h
@@ -83,6 +83,9 @@ enum PredefinedDeclIDs {
/// The internal '__type_pack_element' template.
PREDEF_DECL_TYPE_PACK_ELEMENT_ID,
+ /// The
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -309,7 +309,10 @@ enum BuiltinTemplateKind : int {
BTK__make_integer_seq,
/// This names the __type_pack_element BuiltinTemplateDecl.
- BTK__type_pack_element
+ BTK__type_pack_element,
+
+ /// This names the __type_list_dedup BuiltinTemplateDecl.
+ BTK__type_list_dedup,
ilya-biryukov wrote:
On the argument of adding `VariadicTransformType`, maybe it's useful for
something, but I definitely don't see why introducing a new bulitin template is
more complicated than introducing a new type trait. I think they're quite on
par in terms of complexity and we should be looking at what fits the language
design better (see first two bullet points above on why I feel it's a better
choice for the language).
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
@@ -309,7 +309,10 @@ enum BuiltinTemplateKind : int {
BTK__make_integer_seq,
/// This names the __type_pack_element BuiltinTemplateDecl.
- BTK__type_pack_element
+ BTK__type_pack_element,
+
+ /// This names the __type_list_dedup BuiltinTemplateDecl.
+ BTK__type_list_dedup,
ilya-biryukov wrote:
I have mentioned that in the RFC, see [alternatives
considered](https://discourse.llvm.org/t/rfc-adding-builtin-for-deduplicating-type-lists/80986#p-328944-alternative-considered-type-trait-4)
section.
I think the builtin template is better because it:
- provides the most obvious semantics for type aliases and avoids confusion,
- avoids the need for instantiation of the underlying template with
non-duplicated list types,
- is simpler to implement.
https://github.com/llvm/llvm-project/pull/106730
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -2133,6 +2142,18 @@ void
CXXRecordDecl::completeDefinition(CXXFinalOverriderMap *FinalOverriders) {
for (conversion_iterator I = conversion_begin(), E = conversion_end();
I != E; ++I)
I.setAccess((*I)->getAccess());
+
+ ASTContext &Context = getASTContext();
+ if (!Context.getLangOpts().CPlusPlus20 && hasUserDeclaredConstructor()) {
+// Diagnose any aggregate behavior changes in C++20
+for (field_iterator I = field_begin(), E = field_end(); I != E; ++I) {
+ if (const auto *attr = I->getAttr()) {
+Context.getDiagnostics().Report(
+getLocation(), diag::warn_cxx20_compat_aggregate_init_with_ctors)
ilya-biryukov wrote:
Should we add a new warning here?
`"Attribute [[clang:...]] will have no effect in C++20 as the type will be
non-aggregate due to user-declared constructors"`.
The wording for an existing attribute suggests there is already some
initialization of this type happening, which is not necessarily the case.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1472,3 +1472,25 @@ template struct Outer {
};
};
Outer::Inner outerinner;
+
+void aggregate() {
ilya-biryukov wrote:
Could we add examples from @AaronBallman's [RFC
comment](https://discourse.llvm.org/t/rfc-add-clang-attribute-to-ensure-that-fields-are-initialized-explicitly/80626/20)?
This raises all the same interesting questions that he mentioned, I am leaning
towards limiting the support on any cases that require significant work to
support (e.g. empty struct, zero-width bitfields or all bitfields altogether)
in the initial implementation and simply filing bugs to address those
limitations later.
I do feel we should warn that an attribute has no effect on those fields /
structs if we choose to do so and test those warnings show up.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov commented: I think there are some good parallel disscussions happening in the RFC, but despite their outcomes, we could probably update the PR to capture current behavior in those interesting cases. I left a few comments along those lines, PTAL. https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
ilya-biryukov wrote: > To better understand the motivation, I have two questions: > > * Why do you give Clang module maps describing modules that are already > described in PCM files? Unless the module map describes some other top-level > module that's yet to be built, the information it provides is redundant. Is > this impossible to detect in your build system? It's possible, but much more complicated to do at build system level and it would be much easier to solve this in the compiler. I want to be very clear that we also commit to support this flag while we are using it, and to remove it when we stop doing so. So if it causes any issues, we're there to help and take on the maintenance burden. The build system is Bazel, the code is available upstream too, btw. Although the specific code that runs modules is not enabled by default, and, AFAIK, Google internally is the only user of that. Definitely the only user that cares about the scale that causes these issues. > * Why do you need to wait for 64-bit source locations to be the default > upstream? Is the concern that it's not a well-tested configuration right now > and you don't want to shoulder the burden of qualifying that mode? The biggest concern that we have for 64bit source location is performance costs. We expect some of our compile actions to time out or run out of memory after the change (because the added overhead is significant), so we would like to budget for it and make sure we get a chance to test it in advance of the upstream defaults actually changing. All of that takes time, so this change is intended to give us a way out the corner that we're in until 64 bit source locations are in. > This patch looks good to me, but I think it'd be worth pursuing making this > the default behavior, i.e. investigating the test failures and at least > understanding what exactly is going on. I can take another look, but my vague memory from the investigation I made last time I checked, is that there were some module map shadowing semantics that we could not support unless we read the module map before loading the PCM. We do not use that internally and rely on the fact that each module name is uniquely identified by its module map, hence we can use the flag without giving it too much thought. I will need to refresh my memory, but I believe the conclusion I had at the time was that this shadowing is fundamentally incompatible with loading the module maps later than PCMs, because it affects which PCMs need to loaded. (The conclusions are a little rushed, I may misremember, will take another look next week). That was another reason why I felt that having this as a `-cc1` flag is the only reasonable option, otherwise the interface just gets way too complicated. https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Modules] Add -cc1 -flate-module-map-file to load module maps after PCMs (PR #88893)
ilya-biryukov wrote: @ChuanqiXu9 thanks for taking a look! Makes sense to give others a chance to chime in, I will wait for more comments. https://github.com/llvm/llvm-project/pull/88893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
ilya-biryukov wrote: I still need to add code that preserves source locations for template arguments and tests for it, but otherwise this is ready for review and I would like to get some initial feedback. The RFC did not get any comments so far, but also happy to discuss the general design there. https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in template arguments (PR #106730)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
https://github.com/ilya-biryukov ready_for_review https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/106730
>From a46885df62ff64f355abb010f778d84309acd10f Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 23 Aug 2024 17:27:26 +0200
Subject: [PATCH 1/3] [Clang] Add __type_list_dedup builtin to deduplicate
types in template arguments
This allows to deduplicate the type lists efficiently in C++. It is
possible to achieve the same effect via template metaprogramming, but
performance of the resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase
and this builtin allows to have significant savings (up to 25% of
compile time on targets that already take 3 minutes to compile).
The same builtin is also used widely enough in the codebase that we
expect a savings from a long tail of uses, too, although it is hard to
put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the
exsisting machinery for builtin templates.
---
.../clangd/unittests/FindTargetTests.cpp | 6
clang/include/clang/AST/ASTContext.h | 11 ++
clang/include/clang/AST/DeclID.h | 3 ++
clang/include/clang/Basic/Builtins.h | 5 ++-
clang/lib/AST/ASTContext.cpp | 8 +
clang/lib/AST/ASTImporter.cpp | 3 ++
clang/lib/AST/DeclTemplate.cpp| 31
clang/lib/Lex/PPMacroExpansion.cpp| 1 +
clang/lib/Sema/SemaLookup.cpp | 7 +++-
clang/lib/Sema/SemaTemplate.cpp | 36 +--
clang/lib/Serialization/ASTReader.cpp | 6
clang/lib/Serialization/ASTWriter.cpp | 3 ++
.../test/Import/builtin-template/Inputs/S.cpp | 7
clang/test/Import/builtin-template/test.cpp | 10 +-
clang/test/PCH/type_list_dedup.cpp| 20 +++
.../SemaTemplate/temp-param-list-dedup.cpp| 33 +
16 files changed, 185 insertions(+), 5 deletions(-)
create mode 100644 clang/test/PCH/type_list_dedup.cpp
create mode 100644 clang/test/SemaTemplate/temp-param-list-dedup.cpp
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 3220a5a6a98250..d9f788cbf2a8a2 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -731,6 +731,12 @@ TEST_F(TargetDeclTest, BuiltinTemplates) {
using type_pack_element = [[__type_pack_element]];
)cpp";
EXPECT_DECLS("TemplateSpecializationTypeLoc", );
+
+ Code = R"cpp(
+template Templ, class... Types>
+using type_list_dedup = [[__type_list_dedup]];
+ )cpp";
+ EXPECT_DECLS("TemplateSpecializationTypeLoc", );
}
TEST_F(TargetDeclTest, MemberOfTemplate) {
diff --git a/clang/include/clang/AST/ASTContext.h
b/clang/include/clang/AST/ASTContext.h
index 89bb5768dbd40d..1960ec1e99f652 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -401,6 +401,9 @@ class ASTContext : public RefCountedBase {
/// The identifier '__type_pack_element'.
mutable IdentifierInfo *TypePackElementName = nullptr;
+ /// The identifier '__type_list_dedup'.
+ mutable IdentifierInfo *TypeListDedupName = nullptr;
+
QualType ObjCConstantStringType;
mutable RecordDecl *CFConstantStringTagDecl = nullptr;
mutable TypedefDecl *CFConstantStringTypeDecl = nullptr;
@@ -608,6 +611,7 @@ class ASTContext : public RefCountedBase {
mutable ExternCContextDecl *ExternCContext = nullptr;
mutable BuiltinTemplateDecl *MakeIntegerSeqDecl = nullptr;
mutable BuiltinTemplateDecl *TypePackElementDecl = nullptr;
+ mutable BuiltinTemplateDecl *TypeListDedupDecl = nullptr;
/// The associated SourceManager object.
SourceManager &SourceMgr;
@@ -1115,6 +1119,7 @@ class ASTContext : public RefCountedBase {
ExternCContextDecl *getExternCContextDecl() const;
BuiltinTemplateDecl *getMakeIntegerSeqDecl() const;
BuiltinTemplateDecl *getTypePackElementDecl() const;
+ BuiltinTemplateDecl *getTypeListDedupDecl() const;
// Builtin Types.
CanQualType VoidTy;
@@ -2006,6 +2011,12 @@ class ASTContext : public RefCountedBase {
return TypePackElementName;
}
+ IdentifierInfo *getTypeListDedupName() const {
+if (!TypeListDedupName)
+ TypeListDedupName = &Idents.get("__type_list_dedup");
+return TypeListDedupName;
+ }
+
/// Retrieve the Objective-C "instancetype" type, if already known;
/// otherwise, returns a NULL type;
QualType getObjCInstanceType() {
diff --git a/clang/include/clang/AST/DeclID.h b/clang/include/clang/AST/DeclID.h
index 81454a247229f5..41cecf1b8a9ebf 100644
--- a/clang/include/clang/AST/DeclID.h
+++ b/clang/include/clang/AST/DeclID.h
@@ -83,6 +83,9 @@ enum PredefinedDeclIDs {
/// The internal '__type_pack_element' template.
PREDEF_DECL_TYPE_PACK_ELEMENT_ID,
+ /// The
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/106730
>From a46885df62ff64f355abb010f778d84309acd10f Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 23 Aug 2024 17:27:26 +0200
Subject: [PATCH 1/3] [Clang] Add __type_list_dedup builtin to deduplicate
types in template arguments
This allows to deduplicate the type lists efficiently in C++. It is
possible to achieve the same effect via template metaprogramming, but
performance of the resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase
and this builtin allows to have significant savings (up to 25% of
compile time on targets that already take 3 minutes to compile).
The same builtin is also used widely enough in the codebase that we
expect a savings from a long tail of uses, too, although it is hard to
put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the
exsisting machinery for builtin templates.
---
.../clangd/unittests/FindTargetTests.cpp | 6
clang/include/clang/AST/ASTContext.h | 11 ++
clang/include/clang/AST/DeclID.h | 3 ++
clang/include/clang/Basic/Builtins.h | 5 ++-
clang/lib/AST/ASTContext.cpp | 8 +
clang/lib/AST/ASTImporter.cpp | 3 ++
clang/lib/AST/DeclTemplate.cpp| 31
clang/lib/Lex/PPMacroExpansion.cpp| 1 +
clang/lib/Sema/SemaLookup.cpp | 7 +++-
clang/lib/Sema/SemaTemplate.cpp | 36 +--
clang/lib/Serialization/ASTReader.cpp | 6
clang/lib/Serialization/ASTWriter.cpp | 3 ++
.../test/Import/builtin-template/Inputs/S.cpp | 7
clang/test/Import/builtin-template/test.cpp | 10 +-
clang/test/PCH/type_list_dedup.cpp| 20 +++
.../SemaTemplate/temp-param-list-dedup.cpp| 33 +
16 files changed, 185 insertions(+), 5 deletions(-)
create mode 100644 clang/test/PCH/type_list_dedup.cpp
create mode 100644 clang/test/SemaTemplate/temp-param-list-dedup.cpp
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 3220a5a6a98250..d9f788cbf2a8a2 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -731,6 +731,12 @@ TEST_F(TargetDeclTest, BuiltinTemplates) {
using type_pack_element = [[__type_pack_element]];
)cpp";
EXPECT_DECLS("TemplateSpecializationTypeLoc", );
+
+ Code = R"cpp(
+template Templ, class... Types>
+using type_list_dedup = [[__type_list_dedup]];
+ )cpp";
+ EXPECT_DECLS("TemplateSpecializationTypeLoc", );
}
TEST_F(TargetDeclTest, MemberOfTemplate) {
diff --git a/clang/include/clang/AST/ASTContext.h
b/clang/include/clang/AST/ASTContext.h
index 89bb5768dbd40d..1960ec1e99f652 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -401,6 +401,9 @@ class ASTContext : public RefCountedBase {
/// The identifier '__type_pack_element'.
mutable IdentifierInfo *TypePackElementName = nullptr;
+ /// The identifier '__type_list_dedup'.
+ mutable IdentifierInfo *TypeListDedupName = nullptr;
+
QualType ObjCConstantStringType;
mutable RecordDecl *CFConstantStringTagDecl = nullptr;
mutable TypedefDecl *CFConstantStringTypeDecl = nullptr;
@@ -608,6 +611,7 @@ class ASTContext : public RefCountedBase {
mutable ExternCContextDecl *ExternCContext = nullptr;
mutable BuiltinTemplateDecl *MakeIntegerSeqDecl = nullptr;
mutable BuiltinTemplateDecl *TypePackElementDecl = nullptr;
+ mutable BuiltinTemplateDecl *TypeListDedupDecl = nullptr;
/// The associated SourceManager object.
SourceManager &SourceMgr;
@@ -1115,6 +1119,7 @@ class ASTContext : public RefCountedBase {
ExternCContextDecl *getExternCContextDecl() const;
BuiltinTemplateDecl *getMakeIntegerSeqDecl() const;
BuiltinTemplateDecl *getTypePackElementDecl() const;
+ BuiltinTemplateDecl *getTypeListDedupDecl() const;
// Builtin Types.
CanQualType VoidTy;
@@ -2006,6 +2011,12 @@ class ASTContext : public RefCountedBase {
return TypePackElementName;
}
+ IdentifierInfo *getTypeListDedupName() const {
+if (!TypeListDedupName)
+ TypeListDedupName = &Idents.get("__type_list_dedup");
+return TypeListDedupName;
+ }
+
/// Retrieve the Objective-C "instancetype" type, if already known;
/// otherwise, returns a NULL type;
QualType getObjCInstanceType() {
diff --git a/clang/include/clang/AST/DeclID.h b/clang/include/clang/AST/DeclID.h
index 81454a247229f5..41cecf1b8a9ebf 100644
--- a/clang/include/clang/AST/DeclID.h
+++ b/clang/include/clang/AST/DeclID.h
@@ -83,6 +83,9 @@ enum PredefinedDeclIDs {
/// The internal '__type_pack_element' template.
PREDEF_DECL_TYPE_PACK_ELEMENT_ID,
+ /// The
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1472,3 +1472,25 @@ template struct Outer {
};
};
Outer::Inner outerinner;
+
+void aggregate() {
+ struct B {
+[[clang::explicit_init]] int f1;
+ };
+
+ struct S : B { // expected-warning {{uninitialized}}
+int f2;
+int f3 [[clang::explicit_init]];
+ };
+
+#if __cplusplus >= 202002L
+ S a({}, 0); // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}}
+#endif
+ S b{.f3 = 1}; // expected-warning {{'f1' is left uninitialized}}
+ S c{.f2 = 5}; // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}} expected-warning {{'f3' is left
uninitialized}}
+ c = {{}, 0}; // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}}
+ S d; // expected-warning {{uninitialized}} expected-note {{constructor}}
ilya-biryukov wrote:
Okay, fair point, and I agree.
I still feel that using this consistently in C++17 would be a challenge, but
people are also free to disable this warning in C++17 **or** if they like
multi-line initialization in C++20.
To give the discussions a more useful spin: I suspect we could remove some of
these warnings if we are willing to pay for a more advanced analysis in Clang.
It's technically possible to check that those fields of a struct are
initialized before being read in certain scenarios. The most obvious one is
when all initializations are inside a single function. Currently, we would warn
when the struct is created, but with the more advanced analysis we may remove
this restriction in "obviously safe" cases, which is a strict improvement.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1419,6 +1419,28 @@ is not specified.
}];
}
+def ExplicitInitDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``clang::explicit_init`` attribute indicates that the field must be
+initialized explicitly by the caller when the class is constructed.
ilya-biryukov wrote:
Thanks! LG!
> we might want to forbid its usage on non-aggregates entirely.
I guess we could simply warn if the class in which the field is used is
non-aggregate, but that entails the problems that were raised with the changed
semantics of what's aggregate between C++17 and C++20 (especially if people use
`Werror`).
I guess having a warning that is off by default is a no-brainer, it clearly
adds value **if** people want it. But it might also happen that nobody uses it
and the work for it is wasted.
Or we could simply document that the attribute is ignored for non-aggregate
types in the documentation.
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -146,9 +148,24 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
+auto QualifierIsNamepsaceOrNone = [&DRE]() {
+ const auto *Qual = DRE->getQualifier();
+ if (!Qual)
+return true;
+ switch (Qual->getKind()) {
+ case NestedNameSpecifier::Namespace:
+ case NestedNameSpecifier::NamespaceAlias:
+ case NestedNameSpecifier::Global:
+return true;
+ case NestedNameSpecifier::TypeSpec:
+ case NestedNameSpecifier::TypeSpecWithTemplate:
+ case NestedNameSpecifier::Super:
+ case NestedNameSpecifier::Identifier:
+return false;
+ }
ilya-biryukov wrote:
Yeah, we don't use `-Werror`, though, and don't use the same compiler.
So missing the error during development is not unheard of, and I don't think
our precommit CI catches that either.
This is why `llvm_unreachable` still makes sense from my perspective, I always
put it as an extra layer of defense.
As for `default: ... `, we have an explicit [rule in LLVM Style
Guide](https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations)
that asks not to do that for fully-covered switches.
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
ilya-biryukov wrote: RFC for this change: https://discourse.llvm.org/t/rfc-adding-builtin-for-deduplicating-type-lists/80986 https://github.com/llvm/llvm-project/pull/106730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -146,9 +148,24 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
+auto QualifierIsNamepsaceOrNone = [&DRE]() {
ilya-biryukov wrote:
NIT: Maybe make this a helper function instead?
Having a lambda reduces the scope of visibility, but we also loose the benefits
of making the body of our function smaller. I tend to prefer smaller functions,
but up to you.
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -534,6 +534,8 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
ilya-biryukov wrote:
so it was a good test case! thanks, LG!
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -146,9 +148,24 @@ class ASTWalker : public RecursiveASTVisitor {
//
// If it's an enum constant, it must be due to prior decl. Report
references
// to it when qualifier isn't a type.
-if (llvm::isa(FD)) {
- if (!DRE->getQualifier() || DRE->getQualifier()->getAsNamespace())
-report(DRE->getLocation(), FD);
+auto QualifierIsNamepsaceOrNone = [&DRE]() {
+ const auto *Qual = DRE->getQualifier();
+ if (!Qual)
+return true;
+ switch (Qual->getKind()) {
+ case NestedNameSpecifier::Namespace:
+ case NestedNameSpecifier::NamespaceAlias:
+ case NestedNameSpecifier::Global:
+return true;
+ case NestedNameSpecifier::TypeSpec:
+ case NestedNameSpecifier::TypeSpecWithTemplate:
+ case NestedNameSpecifier::Super:
+ case NestedNameSpecifier::Identifier:
+return false;
+ }
ilya-biryukov wrote:
NIT: maybe add `llvm_unreachable` to make sure we get some error messages if
someone adds a new kind of nested-specifier in the future?
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/ilya-biryukov approved this pull request. Still LGTM. See one minor comment about the code style, but up to you if you want to apply it or not. https://github.com/llvm/llvm-project/pull/106706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
https://github.com/ilya-biryukov updated
https://github.com/llvm/llvm-project/pull/106730
>From 0151fbf9a8cea2d1c60dc399f088258dd94ad562 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 23 Aug 2024 17:27:26 +0200
Subject: [PATCH 1/2] [Clang] Add __type_list_dedup builtin to deduplicate
types in template arguments
This allows to deduplicate the type lists efficiently in C++. It is
possible to achieve the same effect via template metaprogramming, but
performance of the resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase
and this builtin allows to have significant savings (up to 25% of
compile time on targets that already take 3 minutes to compile).
The same builtin is also used widely enough in the codebase that we
expect a savings from a long tail of uses, too, although it is hard to
put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the
exsisting machinery for builtin templates.
---
.../clangd/unittests/FindTargetTests.cpp | 6
clang/include/clang/AST/ASTContext.h | 11 ++
clang/include/clang/AST/DeclID.h | 5 ++-
clang/include/clang/Basic/Builtins.h | 5 ++-
clang/lib/AST/ASTContext.cpp | 8 +
clang/lib/AST/ASTImporter.cpp | 3 ++
clang/lib/AST/DeclTemplate.cpp| 31
clang/lib/Lex/PPMacroExpansion.cpp| 1 +
clang/lib/Sema/SemaLookup.cpp | 7 +++-
clang/lib/Sema/SemaTemplate.cpp | 36 +--
clang/lib/Serialization/ASTReader.cpp | 6
clang/lib/Serialization/ASTWriter.cpp | 3 ++
.../test/Import/builtin-template/Inputs/S.cpp | 7
clang/test/Import/builtin-template/test.cpp | 10 +-
clang/test/PCH/type_list_dedup.cpp| 20 +++
.../SemaTemplate/temp-param-list-dedup.cpp| 33 +
16 files changed, 186 insertions(+), 6 deletions(-)
create mode 100644 clang/test/PCH/type_list_dedup.cpp
create mode 100644 clang/test/SemaTemplate/temp-param-list-dedup.cpp
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 3220a5a6a98250..d9f788cbf2a8a2 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -731,6 +731,12 @@ TEST_F(TargetDeclTest, BuiltinTemplates) {
using type_pack_element = [[__type_pack_element]];
)cpp";
EXPECT_DECLS("TemplateSpecializationTypeLoc", );
+
+ Code = R"cpp(
+template Templ, class... Types>
+using type_list_dedup = [[__type_list_dedup]];
+ )cpp";
+ EXPECT_DECLS("TemplateSpecializationTypeLoc", );
}
TEST_F(TargetDeclTest, MemberOfTemplate) {
diff --git a/clang/include/clang/AST/ASTContext.h
b/clang/include/clang/AST/ASTContext.h
index 58a820508da42b..3226104ecaee5c 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -400,6 +400,9 @@ class ASTContext : public RefCountedBase {
/// The identifier '__type_pack_element'.
mutable IdentifierInfo *TypePackElementName = nullptr;
+ /// The identifier '__type_list_dedup'.
+ mutable IdentifierInfo *TypeListDedupName = nullptr;
+
QualType ObjCConstantStringType;
mutable RecordDecl *CFConstantStringTagDecl = nullptr;
mutable TypedefDecl *CFConstantStringTypeDecl = nullptr;
@@ -607,6 +610,7 @@ class ASTContext : public RefCountedBase {
mutable ExternCContextDecl *ExternCContext = nullptr;
mutable BuiltinTemplateDecl *MakeIntegerSeqDecl = nullptr;
mutable BuiltinTemplateDecl *TypePackElementDecl = nullptr;
+ mutable BuiltinTemplateDecl *TypeListDedupDecl = nullptr;
/// The associated SourceManager object.
SourceManager &SourceMgr;
@@ -1114,6 +1118,7 @@ class ASTContext : public RefCountedBase {
ExternCContextDecl *getExternCContextDecl() const;
BuiltinTemplateDecl *getMakeIntegerSeqDecl() const;
BuiltinTemplateDecl *getTypePackElementDecl() const;
+ BuiltinTemplateDecl *getTypeListDedupDecl() const;
// Builtin Types.
CanQualType VoidTy;
@@ -1993,6 +1998,12 @@ class ASTContext : public RefCountedBase {
return TypePackElementName;
}
+ IdentifierInfo *getTypeListDedupName() const {
+if (!TypeListDedupName)
+ TypeListDedupName = &Idents.get("__type_list_dedup");
+return TypeListDedupName;
+ }
+
/// Retrieve the Objective-C "instancetype" type, if already known;
/// otherwise, returns a NULL type;
QualType getObjCInstanceType() {
diff --git a/clang/include/clang/AST/DeclID.h b/clang/include/clang/AST/DeclID.h
index e5e27389fac60d..81d4df530e6cf3 100644
--- a/clang/include/clang/AST/DeclID.h
+++ b/clang/include/clang/AST/DeclID.h
@@ -84,13 +84,16 @@ enum PredefinedDeclIDs {
/// The internal '__type_pack_element' template.
PREDEF_DECL_TYPE_PACK_ELEMENT_ID = 17,
+
[clang] [clang-tools-extra] [Clang] Add __type_list_dedup builtin to deduplicate types in templat… (PR #106730)
https://github.com/ilya-biryukov created
https://github.com/llvm/llvm-project/pull/106730
…e arguments
This allows to deduplicate the type lists efficiently in C++. It is possible to
achieve the same effect via template metaprogramming, but performance of the
resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase and
this builtin allows to have significant savings (up to 25% of compile time on
targets that already take 3 minutes to compile). The same builtin is also used
widely enough in the codebase that we expect a savings from a long tail of
uses, too, although it is hard to put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the exsisting
machinery for builtin templates.
>From 0151fbf9a8cea2d1c60dc399f088258dd94ad562 Mon Sep 17 00:00:00 2001
From: Ilya Biryukov
Date: Fri, 23 Aug 2024 17:27:26 +0200
Subject: [PATCH] [Clang] Add __type_list_dedup builtin to deduplicate types in
template arguments
This allows to deduplicate the type lists efficiently in C++. It is
possible to achieve the same effect via template metaprogramming, but
performance of the resulting code is much lower than in the compiler.
We have observed that this code is quite common in our internal codebase
and this builtin allows to have significant savings (up to 25% of
compile time on targets that already take 3 minutes to compile).
The same builtin is also used widely enough in the codebase that we
expect a savings from a long tail of uses, too, although it is hard to
put an estimate on this number in advance.
The implementation aims to be as simple as possible and relies on the
exsisting machinery for builtin templates.
---
.../clangd/unittests/FindTargetTests.cpp | 6
clang/include/clang/AST/ASTContext.h | 11 ++
clang/include/clang/AST/DeclID.h | 5 ++-
clang/include/clang/Basic/Builtins.h | 5 ++-
clang/lib/AST/ASTContext.cpp | 8 +
clang/lib/AST/ASTImporter.cpp | 3 ++
clang/lib/AST/DeclTemplate.cpp| 31
clang/lib/Lex/PPMacroExpansion.cpp| 1 +
clang/lib/Sema/SemaLookup.cpp | 7 +++-
clang/lib/Sema/SemaTemplate.cpp | 36 +--
clang/lib/Serialization/ASTReader.cpp | 6
clang/lib/Serialization/ASTWriter.cpp | 3 ++
.../test/Import/builtin-template/Inputs/S.cpp | 7
clang/test/Import/builtin-template/test.cpp | 10 +-
clang/test/PCH/type_list_dedup.cpp| 20 +++
.../SemaTemplate/temp-param-list-dedup.cpp| 33 +
16 files changed, 186 insertions(+), 6 deletions(-)
create mode 100644 clang/test/PCH/type_list_dedup.cpp
create mode 100644 clang/test/SemaTemplate/temp-param-list-dedup.cpp
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index 3220a5a6a98250..d9f788cbf2a8a2 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -731,6 +731,12 @@ TEST_F(TargetDeclTest, BuiltinTemplates) {
using type_pack_element = [[__type_pack_element]];
)cpp";
EXPECT_DECLS("TemplateSpecializationTypeLoc", );
+
+ Code = R"cpp(
+template Templ, class... Types>
+using type_list_dedup = [[__type_list_dedup]];
+ )cpp";
+ EXPECT_DECLS("TemplateSpecializationTypeLoc", );
}
TEST_F(TargetDeclTest, MemberOfTemplate) {
diff --git a/clang/include/clang/AST/ASTContext.h
b/clang/include/clang/AST/ASTContext.h
index 58a820508da42b..3226104ecaee5c 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -400,6 +400,9 @@ class ASTContext : public RefCountedBase {
/// The identifier '__type_pack_element'.
mutable IdentifierInfo *TypePackElementName = nullptr;
+ /// The identifier '__type_list_dedup'.
+ mutable IdentifierInfo *TypeListDedupName = nullptr;
+
QualType ObjCConstantStringType;
mutable RecordDecl *CFConstantStringTagDecl = nullptr;
mutable TypedefDecl *CFConstantStringTypeDecl = nullptr;
@@ -607,6 +610,7 @@ class ASTContext : public RefCountedBase {
mutable ExternCContextDecl *ExternCContext = nullptr;
mutable BuiltinTemplateDecl *MakeIntegerSeqDecl = nullptr;
mutable BuiltinTemplateDecl *TypePackElementDecl = nullptr;
+ mutable BuiltinTemplateDecl *TypeListDedupDecl = nullptr;
/// The associated SourceManager object.
SourceManager &SourceMgr;
@@ -1114,6 +1118,7 @@ class ASTContext : public RefCountedBase {
ExternCContextDecl *getExternCContextDecl() const;
BuiltinTemplateDecl *getMakeIntegerSeqDecl() const;
BuiltinTemplateDecl *getTypePackElementDecl() const;
+ BuiltinTemplateDecl *getTypeListDedupDecl() const;
// Builtin Types.
CanQualType VoidTy;
@@ -1993,6 +1998,12 @@ class ASTContext : public
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/ilya-biryukov approved this pull request. LGTM. See a small suggestion about adding another test too. https://github.com/llvm/llvm-project/pull/106706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106706 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [include-cleaner] Report refs for enum constants used through namespace aliases (PR #106706)
@@ -534,6 +534,8 @@ TEST(WalkAST, Enums) {
testWalk(R"(namespace ns { enum E { A = 42 }; }
struct S { using ns::E::A; };)",
"int e = S::^A;");
+ testWalk(R"(namespace ns { enum E { $explicit^A = 42 }; })",
+ "namespace z = ns; int e = z::^A;");
ilya-biryukov wrote:
Could you add a test for the global specifier too?
```::A``` has a slightly different representation as `NestedNameSpecifier`, so
we might want to make sure this case is also tested. (I **think** the existing
code should work just fine, though).
https://github.com/llvm/llvm-project/pull/106706
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1419,6 +1419,28 @@ is not specified.
}];
}
+def ExplicitInitDocs : Documentation {
+ let Category = DocCatField;
+ let Content = [{
+The ``clang::explicit_init`` attribute indicates that the field must be
+initialized explicitly by the caller when the class is constructed.
ilya-biryukov wrote:
Maybe document that the caveats here?
I could see us exploring the extension of this analysis to make sure the
"explicit_init" fields are all written before they're read, but that's not
what's happening today IIUC.
Another caveat, is that the attribute is ignored on non-aggregate classes.
```cpp
struct NonAgg {
NonAgg() {}
int x [[clang::explicit_init]]; // attribute ignored.
}
```
And maybe also mention that these warnings do not aim to comprehensively
prevent uses of uninitialized memory, but are, rather, supporting tools for
aiding to write code in certain code style if people choose to (e.g. using it
for parameter objects in combination with C++20 designated initializer).
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
@@ -1472,3 +1472,25 @@ template struct Outer {
};
};
Outer::Inner outerinner;
+
+void aggregate() {
+ struct B {
+[[clang::explicit_init]] int f1;
+ };
+
+ struct S : B { // expected-warning {{uninitialized}}
+int f2;
+int f3 [[clang::explicit_init]];
+ };
+
+#if __cplusplus >= 202002L
+ S a({}, 0); // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}}
+#endif
+ S b{.f3 = 1}; // expected-warning {{'f1' is left uninitialized}}
+ S c{.f2 = 5}; // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}} expected-warning {{'f3' is left
uninitialized}}
+ c = {{}, 0}; // expected-warning {{'f1' is left uninitialized}}
expected-warning {{'f3' is left uninitialized}}
+ S d; // expected-warning {{uninitialized}} expected-note {{constructor}}
ilya-biryukov wrote:
This is the only example that I'm torn on because it prohibits writing the very
common code:
```
S d;
d.f1 = 123;
d.f3 = 234;
// There's nothing wrong with this multi-line initialization style ^^^
```
While I'm fully on board with the idea that this warning is helpful in cases
where aggregate initialization is used, I think I would not restrict the code
to use **only** the aggregate initialization.
Especially in C++17 and below, where there is no way to spell the names of the
fields in aggregate init syntax.
WDYT about leaving this out? It would also allow us to get rid of the flag we
need to store in the class altogether.
Or would this make the warning less useful, to an extent where you don't want
to have it?
https://github.com/llvm/llvm-project/pull/102040
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov commented: I hope folks are ok with me chiming in as a reviewer for this. I've left quite a few comments in the RFC and is also supportive of landing this change and happy to invest into supporting it going forward inside our team. https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Add Clang attribute to ensure that fields are initialized explicitly (PR #102040)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/102040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
@@ -453,6 +454,61 @@ TEST_F(SourceManagerTest,
loadedSLocEntryIsInTheSameTranslationUnit) {
#if defined(LLVM_ON_UNIX)
+TEST_F(SourceManagerTest, ResetsIncludeLocMap) {
ilya-biryukov wrote:
Suggestion: add a comment explaining what this aims to test at a more detailed
level. Something like:
```cpp
// Check we clear include loc caches when resetting the source manager.
```
https://github.com/llvm/llvm-project/pull/106241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/ilya-biryukov approved this pull request. LGMT with a small suggestion. Thanks for getting the test in! https://github.com/llvm/llvm-project/pull/106241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Cleanup IncludeLocMap (PR #106241)
https://github.com/ilya-biryukov edited https://github.com/llvm/llvm-project/pull/106241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -194,13 +236,15 @@ struct llvm::TimeTraceProfiler {
J.attribute("pid", Pid);
J.attribute("tid", int64_t(Tid));
J.attribute("ts", StartUs);
-if (E.AsyncEvent) {
+if (TimeTraceEventType::AsyncEvent == E.EventType) {
J.attribute("cat", E.Name);
J.attribute("ph", "b");
J.attribute("id", 0);
-} else {
+} else if (E.EventType == TimeTraceEventType::CompleteEvent) {
J.attribute("ph", "X");
J.attribute("dur", DurUs);
+} else { // instant event
+ J.attribute("ph", "i");
ilya-biryukov wrote:
Suggestion: add `assert` checking this invariant.
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -406,34 +450,50 @@ TimeTraceProfilerEntry
*llvm::timeTraceProfilerBegin(StringRef Name,
StringRef Detail) {
if (TimeTraceProfilerInstance != nullptr)
return TimeTraceProfilerInstance->begin(
-std::string(Name), [&]() { return std::string(Detail); }, false);
+std::string(Name), [&]() { return std::string(Detail); },
+TimeTraceEventType::CompleteEvent);
return nullptr;
}
TimeTraceProfilerEntry *
llvm::timeTraceProfilerBegin(StringRef Name,
llvm::function_ref Detail) {
if (TimeTraceProfilerInstance != nullptr)
-return TimeTraceProfilerInstance->begin(std::string(Name), Detail, false);
+return TimeTraceProfilerInstance->begin(std::string(Name), Detail,
+TimeTraceEventType::CompleteEvent);
return nullptr;
}
TimeTraceProfilerEntry *
llvm::timeTraceProfilerBegin(StringRef Name,
llvm::function_ref Metadata)
{
if (TimeTraceProfilerInstance != nullptr)
-return TimeTraceProfilerInstance->begin(std::string(Name), Metadata,
false);
+return TimeTraceProfilerInstance->begin(std::string(Name), Metadata,
+TimeTraceEventType::CompleteEvent);
return nullptr;
}
TimeTraceProfilerEntry *llvm::timeTraceAsyncProfilerBegin(StringRef Name,
StringRef Detail) {
if (TimeTraceProfilerInstance != nullptr)
return TimeTraceProfilerInstance->begin(
-std::string(Name), [&]() { return std::string(Detail); }, true);
+std::string(Name), [&]() { return std::string(Detail); },
+TimeTraceEventType::AsyncEvent);
return nullptr;
}
+void llvm::timeTraceProfilerInsert(StringRef Name, StringRef Detail) {
ilya-biryukov wrote:
NIT: could we rename this to `timeTraceInstantEvent`?
It should make the uses more obvious, right now `Insert` seems to be open for
interpretation.
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -104,6 +105,23 @@ struct llvm::TimeTraceProfilerEntry {
}
};
+struct InProgressEntry {
+ std::unique_ptr Event;
+ std::vector InstantEvents;
+
+ InProgressEntry(TimePointType &&S, TimePointType &&E, std::string &&N,
ilya-biryukov wrote:
Suggestion: removing these constructors and simply creating the
`InProgressEntry` by hand would result in less code and less boilerplate (even
though the usage will span more lines):
```
InProgressEntry E;
E.Event = make_unique<...>
```
Given that we only have two places where it's used, I think we're going to make
things simpler.
(When LLVM switches to C++20 it would also be possible to use designated
initializer syntax and actually get very readable code on a single line too,
but that's a long time in the future).
However, this file already uses constructors, so this approach would be a
little inconsistent.
Feel free to agree or ignore, just a suggestion.
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -83,6 +83,8 @@ namespace llvm {
class raw_pwrite_stream;
+enum class TimeTraceEventType { CompleteEvent, InstantEvent, AsyncEvent };
ilya-biryukov wrote:
It would be useful to add a comment explaining what different event types are
about (e.g. it would mention which fields are ignored by instant events)
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -104,6 +105,23 @@ struct llvm::TimeTraceProfilerEntry {
}
};
+struct InProgressEntry {
+ std::unique_ptr Event;
+ std::vector InstantEvents;
+
+ InProgressEntry(TimePointType &&S, TimePointType &&E, std::string &&N,
+ std::string &&Dt, TimeTraceEventType Et)
+ : Event(std::make_unique(
+std::move(S), std::move(E), std::move(N), std::move(Dt), Et)),
+InstantEvents() {}
+
+ InProgressEntry(TimePointType &&S, TimePointType &&E, std::string &&N,
ilya-biryukov wrote:
I suggested to remove the constructors completely, but also wanted to mention
that it's better to accept by value in constructors, r-value references are
almost always the wrong choice.
If the value is used, users still have a chance to `std::move()` into it to get
efficiency; but they're also allowed to copy in cases where the type is const
or a copy is actually needed.
See https://gcc.godbolt.org/z/9s3oo6xz8 for some examples.
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] Added instant events and marking defered templated instantiation. (PR #103039)
@@ -75,18 +76,18 @@ struct llvm::TimeTraceProfilerEntry {
const std::string Name;
TimeTraceMetadata Metadata;
- const bool AsyncEvent = false;
+ TimeTraceEventType EventType;
ilya-biryukov wrote:
Let's assign the default to avoid accidentally having uninitialized fields
(like we used before).
Both enums and bools can produce those.
`= CompleteEvent` should be an ok choice.
https://github.com/llvm/llvm-project/pull/103039
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
