-
Notifications
You must be signed in to change notification settings - Fork 15k
[clang] Add the candiscard attribute to suppress nodiscard #154943
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
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 |
|---|---|---|
|
|
@@ -3646,6 +3646,13 @@ def Unavailable : InheritableAttr { | |
| let MeaningfulToClassTemplateDefinition = 1; | ||
| } | ||
|
|
||
| def CanDiscard : InheritableAttr { | ||
| let Spellings = [Clang<"candiscard">]; | ||
| let Subjects = SubjectList<[ObjCMethod, FunctionLike, TypedefName]>; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we sure about this subject list? IF you leave any of the above, please make sure that ALL of the FunctionLike has well defined semantics/etc, and that the ObjectiveC types are tested/have well defined semantics. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same subject list as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See: https://clang.llvm.org/doxygen/DeclBase_8cpp_source.html#l01188 IF you want to support all of those, we need to see tests for all of those.I was trying to help limit work by making this apply to functions ONLY, that way we can expand it if/when someone requests it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have added tests for member function pointers. Function references should work the same as functions and function pointers. I could not find any tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that seems reasonable. We should still have function reference tests all the same. IMO, I'm not sure the value is there for having this on pointers/references though. |
||
| let Documentation = [CanDiscardDocs]; | ||
| let SimpleHandler = 1; | ||
| } | ||
|
|
||
| def DiagnoseIf : InheritableAttr { | ||
| // Does not have a [[]] spelling because this attribute requires the ability | ||
| // to parse function arguments but the attribute is not written in the type | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2452,6 +2452,44 @@ use the annotated ``[[nodiscard]]`` constructor or result in an annotated type. | |
| }]; | ||
| } | ||
|
|
||
| def CanDiscardDocs : Documentation { | ||
| let Category = DocCatFunction; | ||
| let Heading = "candiscard"; | ||
| let Content = [{ | ||
| A function whose return type is marked with ``[[nodiscard]]`` generally cannot have | ||
| its return value discarded, even though this may be safe in some rare situations. | ||
| Clang allows an individual function to be marked with ``[[clang::candiscard]]`` | ||
| or ``__attribute__((candiscard))`` to override the effect of a ``[[nodiscard]]`` | ||
| return type. | ||
|
Comment on lines
+2459
to
+2463
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the motivation for exposing it as as a gnu attribute? afaik GCC does not have this behavior |
||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| struct [[nodiscard]] error_info { /*...*/ }; | ||
| error_info enable_missile_safety_mode(); | ||
| [[clang::candiscard]] error_info reload_missiles(); | ||
|
|
||
| void test_missiles() { | ||
| enable_missile_safety_mode(); // diagnoses | ||
| reload_missiles(); // does not diagnose | ||
| } | ||
|
|
||
| Also, a type alias can be marked with ``[[clang::candiscard]]`` to mask the | ||
| effect of ``[[nodiscard]]`` on the underlying type. | ||
|
|
||
| .. code-block:: c++ | ||
|
|
||
| struct [[nodiscard]] error_info { /*...*/ }; | ||
| using informational_error_info [[clang::candiscard]] = error_info; | ||
| error_info enable_missile_safety_mode(); | ||
| informational_error_info reload_missiles(); | ||
|
|
||
| void test_missiles() { | ||
| enable_missile_safety_mode(); // diagnoses | ||
| reload_missiles(); // does not diagnose | ||
| } | ||
| }]; | ||
| } | ||
|
|
||
| def FallthroughDocs : Documentation { | ||
| let Category = DocCatStmt; | ||
| let Heading = "fallthrough"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1632,21 +1632,34 @@ QualType CallExpr::getCallReturnType(const ASTContext &Ctx) const { | |
|
|
||
| std::pair<const NamedDecl *, const WarnUnusedResultAttr *> | ||
| Expr::getUnusedResultAttrImpl(const Decl *Callee, QualType ReturnType) { | ||
| // If the callee is marked nodiscard, return that attribute | ||
| if (Callee != nullptr) | ||
| // If the callee is marked nodiscard, return that attribute for the | ||
| // diagnostic. If the callee is marked candiscard, do not diagnose. | ||
| // If seen on the same level, candiscard beats nodiscard. | ||
| if (Callee != nullptr) { | ||
| if (const auto *A = Callee->getAttr<CanDiscardAttr>()) | ||
| return {nullptr, nullptr}; | ||
| if (const auto *A = Callee->getAttr<WarnUnusedResultAttr>()) | ||
| return {nullptr, A}; | ||
| } | ||
|
|
||
| // If the return type is a struct, union, or enum that is marked nodiscard, | ||
| // then return the return type attribute. | ||
| if (const TagDecl *TD = ReturnType->getAsTagDecl()) | ||
| if (const auto *A = TD->getAttr<WarnUnusedResultAttr>()) | ||
| return {TD, A}; | ||
|
|
||
| // Walk the return type's (chain of) type aliases. The first alias | ||
| // that is marked either nodiscard or candiscard ends the walk. | ||
| for (const auto *TD = ReturnType->getAs<TypedefType>(); TD; | ||
| TD = TD->desugar()->getAs<TypedefType>()) | ||
| TD = TD->desugar()->getAs<TypedefType>()) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make sure we have tests where this is done on a template alias? We want to make sure semantics for this are sensible there too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug #68456 prevents these attributes from working on alias templates |
||
| if (const auto *A = TD->getDecl()->getAttr<CanDiscardAttr>()) | ||
| return {nullptr, nullptr}; | ||
| if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>()) | ||
| return {TD->getDecl(), A}; | ||
| } | ||
|
|
||
| // Check whether the return type's class declaration is marked nodiscard. | ||
| if (const TagDecl *TD = ReturnType->getAsTagDecl()) { | ||
| if (const auto *A = TD->getAttr<CanDiscardAttr>()) | ||
| return {nullptr, nullptr}; | ||
| if (const auto *A = TD->getAttr<WarnUnusedResultAttr>()) | ||
| return {TD, A}; | ||
| } | ||
|
|
||
| return {nullptr, nullptr}; | ||
| } | ||
|
|
||
|
|
||
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.
What do you think of a shorter name
[[clang::discard]]? I have no preference for this。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.
IMO
discardablewould be a good name, but I'm not super attached to any of the names so far.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.
discardablelooks good to me.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.
Where did we land on a name? I'm not particularly attached to anything, but this discussion happened and I didn't see any response to it.
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.
I’m not aware of any discussion but perhaps I just missed it;
discardablesgtm though.