[clang] Warn about deprecated volatile-qualified return types #137899
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (halbi2) ChangesFixes #133380 Full diff: https://github.com/llvm/llvm-project/pull/137899.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 6e7ee8b5506ff..31bdc97d014d6 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -5056,13 +5056,13 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
S.Diag(DeclType.Loc, diag::err_func_returning_qualified_void) << T;
} else
diagnoseRedundantReturnTypeQualifiers(S, T, D, chunkIndex);
-
- // C++2a [dcl.fct]p12:
- // A volatile-qualified return type is deprecated
- if (T.isVolatileQualified() && S.getLangOpts().CPlusPlus20)
- S.Diag(DeclType.Loc, diag::warn_deprecated_volatile_return) << T;
}
+ // C++2a [dcl.fct]p12:
+ // A volatile-qualified return type is deprecated
+ if (T.isVolatileQualified() && S.getLangOpts().CPlusPlus20)
+ S.Diag(DeclType.Loc, diag::warn_deprecated_volatile_return) << T;
+
// Objective-C ARC ownership qualifiers are ignored on the function
// return type (by type canonicalization). Complain if this attribute
// was written here.
diff --git a/clang/test/CXX/expr/expr.const/p2-0x.cpp b/clang/test/CXX/expr/expr.const/p2-0x.cpp
index df5ce108aca82..c6c3381be5523 100644
--- a/clang/test/CXX/expr/expr.const/p2-0x.cpp
+++ b/clang/test/CXX/expr/expr.const/p2-0x.cpp
@@ -356,7 +356,7 @@ namespace LValueToRValue {
// - a non-volatile glvalue of literal type that refers to a non-volatile
// temporary object whose lifetime has not ended, initialized with a
// constant expression;
- constexpr volatile S f() { return S(); }
+ constexpr volatile S f() { return S(); } // cxx20-warning {{volatile-qualified return type 'volatile S' is deprecated}}
static_assert(f().i, ""); // expected-error {{constant expression}} expected-note {{read of volatile-qualified type}}
static_assert(((volatile const S&&)(S)0).i, ""); // expected-error {{constant expression}} expected-note {{read of volatile-qualified type}}
}
diff --git a/clang/test/SemaCXX/deprecated.cpp b/clang/test/SemaCXX/deprecated.cpp
index a24b40d8e622a..061fa8b54dff1 100644
--- a/clang/test/SemaCXX/deprecated.cpp
+++ b/clang/test/SemaCXX/deprecated.cpp
@@ -231,6 +231,13 @@ namespace DeprecatedVolatile {
a = c = a;
b += a;
}
+
+ volatile struct amber jurassic();
+ // cxx20-warning@-1 {{volatile-qualified return type 'volatile struct amber' is deprecated}}
+ void trex(volatile short left_arm, volatile struct amber right_arm);
+ // cxx20-warning@-1 {{volatile-qualified parameter type 'volatile short' is deprecated}}
+ // cxx20-warning@-1 {{volatile-qualified parameter type 'volatile struct amber' is deprecated}}
+ void fly(volatile struct pterosaur* pteranodon);
}
namespace ArithConv {
|
Fznamznon
reviewed
Apr 30, 2025
| // C++2a [dcl.fct]p12: | ||
| // A volatile-qualified return type is deprecated | ||
| if (T.isVolatileQualified() && S.getLangOpts().CPlusPlus20) | ||
| S.Diag(DeclType.Loc, diag::warn_deprecated_volatile_return) << T; |
There was a problem hiding this comment.
I see there is another place that issues warn_deprecated_volatile_return in Sema::CheckFunctionReturnType. I wonder why doesn't it suffice?
There was a problem hiding this comment.
I understand that CheckFunctionReturnType is used when checking the type of a function like using T = volatile int(); but this codepath here is used when checking a function declaration instead.
There was a problem hiding this comment.
ok, thanks for clarifying
| @@ -231,6 +231,13 @@ namespace DeprecatedVolatile { | |||
| a = c = a; | |||
| b += a; | |||
| } | |||
|
|
|||
| volatile struct amber jurassic(); | |||
There was a problem hiding this comment.
As I can see, if the return type is not a struct/class, the warning is issued. Could you please add a test case with volatile qualifier applied to a basic type (for example int) and make sure there is no double diagnostic issued now?
There was a problem hiding this comment.
Lines 207-215 are that, are they not?
There was a problem hiding this comment.
Yeah, these are what I meant. Thanks!
shafik
reviewed
Apr 30, 2025
|
@shafik I have changed the commit message, can you please look again? |
I see the commit 238d737 which adds the improved summary but I don't see at the top of this page in the summary. As long as this is what ends up as the commit message that is fine but it is better to just edit the summary at the top of the page so the reviewers can see it up front. Thank you! |
shafik
approved these changes
May 5, 2025
The old codepath in GetFullTypeForDeclarator was under "if (not a class type)" so that it failed to warn for class types. Move the diagnostic outside of the "if" so that it warns in the proper situations. Fixes llvm#133380
|
@halbi2 Do you need us to merge that for you? |
|
@cor3ntin yes, please. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/11290 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old codepath in GetFullTypeForDeclarator was under "if (not a class type)"
so that it failed to warn for class types. Move the diagnostic outside of the
"if" so that it warns in the proper situations.
Fixes #133380