-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[Clang] Added explanation why is_constructible evaluated to false.
#143309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
ed1f379
ef79556
cb89df9
bd3cefd
172cee8
bd3cfca
bd1e468
124e9f1
e18073b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1945,6 +1945,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) { | |||||
| TypeTrait::UTT_IsCppTriviallyRelocatable) | ||||||
| .Case("is_replaceable", TypeTrait::UTT_IsReplaceable) | ||||||
| .Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable) | ||||||
| .Case("is_constructible", TypeTrait::TT_IsConstructible) | ||||||
| .Default(std::nullopt); | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -1981,8 +1982,14 @@ static ExtractedTypeTraitInfo ExtractTypeTraitFromExpression(const Expr *E) { | |||||
| Trait = StdNameToTypeTrait(Name); | ||||||
| if (!Trait) | ||||||
| return std::nullopt; | ||||||
| for (const auto &Arg : VD->getTemplateArgs().asArray()) | ||||||
| Args.push_back(Arg.getAsType()); | ||||||
| for (const auto &Arg : VD->getTemplateArgs().asArray()) { | ||||||
| if (Arg.getKind() == TemplateArgument::ArgKind::Pack) { | ||||||
| for (const auto &InnerArg : Arg.pack_elements()) | ||||||
| Args.push_back(InnerArg.getAsType()); | ||||||
| } | ||||||
| if (Arg.getKind() == TemplateArgument::ArgKind::Type) | ||||||
|
||||||
| if (Arg.getKind() == TemplateArgument::ArgKind::Type) | |
| else if (Arg.getKind() == TemplateArgument::ArgKind::Type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the kind is neither, we should probably do SOMETHING about it, even if it is just an assert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added assert
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (const auto &ArgTy : Ts) { | |
| for (const QualType &ArgTy : Ts) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete types should have been diagnosed already, so the check for incomplete type is not useful (we do need checks for void and incomplete array types)
https://godbolt.org/z/hc5jrjz51
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it good now? According to my observations when Args contains cv void type it is always evaluated to false, however I can not say the same about incomplete array types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now, for every cv void type, we show a note, but is this a good practice? Or do we want to show it once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I decided to show it once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that Ts is >=1? in size? Maybe early-exit if Ts.empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added early-exit, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IS this a valid assumption here (that this is a type?). Presumably this could be an NTTP pack as well...
Though
getAsTypewould assert for us?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAsType would assert for us