Repository navigation
[clang-tidy] Don't constify params of functions used as values - #236079
Open
yashaggarwal85d wants to merge 1 commit into
Open
yashaggarwal85d wants to merge 1 commit into
yashaggarwal85d wants to merge 1 commit into
Conversation
|
Hello @yashaggarwal85d 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-clang-tidy Author: Yash Aggarwal (yashaggarwal85d) Changesmisc-const-correctness suggests Now, if a function is referenced anywhere in the TU other than as the callee Uses in other TUs still can't be seen. It's also a bit conservative: a Fixes #236012 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
index 4532a27962af29..02a11549dc0ab3 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
@@ -13,6 +13,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include <cassert>
using namespace clang::ast_matchers;
@@ -297,6 +298,10 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
// Offload const-analysis to utility function.
if (isMutated(Variable, LocalScope, Function, Result.Context))
return;
+ // Adding 'const' to a reference parameter changes the function type.
+ if (VC == VariableCategory::Reference && isa<ParmVarDecl>(Variable) &&
+ isUsedAsValue(Function, *Result.Context))
+ return;
const auto Diag = diag(Variable->getBeginLoc(),
"variable %0 of type %1 can be declared 'const'")
@@ -336,6 +341,8 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
registerScope(LocalScope, Result.Context);
if (ScopesCache[LocalScope]->isPointeeMutated(Variable))
return;
+ if (isa<ParmVarDecl>(Variable) && isUsedAsValue(Function, *Result.Context))
+ return;
const auto Diag =
diag(Variable->getBeginLoc(),
"pointee of variable %0 of type %1 can be declared 'const'")
@@ -383,6 +390,28 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
}
}
+bool ConstCorrectnessCheck::isUsedAsValue(const FunctionDecl *Func,
+ ASTContext &Context) {
+ if (!FunctionsUsedAsValue) {
+ FunctionsUsedAsValue.emplace();
+ const auto Matches =
+ match(translationUnitDecl(forEachDescendant(
+ stmt(eachOf(declRefExpr(to(functionDecl())).bind("ref"),
+ callExpr(callee(expr(ignoringParenImpCasts(
+ declRefExpr().bind("callee"))))))))),
+ Context);
+ llvm::SmallPtrSet<const DeclRefExpr *, 32> Callees;
+ for (const auto &Match : Matches)
+ if (const auto *Callee = Match.getNodeAs<DeclRefExpr>("callee"))
+ Callees.insert(Callee);
+ for (const auto &Match : Matches)
+ if (const auto *Ref = Match.getNodeAs<DeclRefExpr>("ref");
+ Ref && !Callees.contains(Ref))
+ FunctionsUsedAsValue->insert(Ref->getDecl()->getCanonicalDecl());
+ }
+ return FunctionsUsedAsValue->contains(Func->getCanonicalDecl());
+}
+
void ConstCorrectnessCheck::registerScope(const Stmt *LocalScope,
ASTContext *Context) {
auto &Analyzer = ScopesCache[LocalScope];
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
index 2e0e2fc67564b3..f996d7e57bf9e5 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
@@ -12,6 +12,7 @@
#include "../ClangTidyCheck.h"
#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
#include "llvm/ADT/DenseSet.h"
+#include <optional>
namespace clang::tidy::misc {
@@ -30,12 +31,18 @@ class ConstCorrectnessCheck : public ClangTidyCheck {
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override { FunctionsUsedAsValue.reset(); }
private:
void registerScope(const Stmt *LocalScope, ASTContext *Context);
bool isMutated(const VarDecl *Variable, const Stmt *Scope,
const FunctionDecl *Func, ASTContext *Context);
+ // Whether the function is referenced other than by being called, e.g. its
+ // address is taken. Its parameter types can't be changed then.
+ bool isUsedAsValue(const FunctionDecl *Func, ASTContext &Context);
+ std::optional<llvm::DenseSet<const Decl *>> FunctionsUsedAsValue;
+
using MutationAnalyzer = std::unique_ptr<ExprMutationAnalyzer>;
llvm::DenseMap<const Stmt *, MutationAnalyzer> ScopesCache;
llvm::DenseSet<SourceLocation> TemplateDiagnosticsCache;
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 310b084c04eb04..02a84a9d3e8be7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -274,6 +274,10 @@ infrastructure are described first, followed by tool-specific sections.
instantiations, where the suggested `const` would break the move for
instantiations with real move semantics.
+ - No longer suggests adding `const` to the pointee or reference of a
+ parameter when the function is used as a value, such as having its
+ address taken, where the changed signature would no longer match.
+
- Fixed an infinite loop in {doc}`misc-multiple-inheritance
<clang-tidy/checks/misc/multiple-inheritance>` when checking a class that
inherits from itself or has a circular inheritance graph.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
index 0a1eaf0e45a57d..d5854db7d8110a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -123,7 +123,11 @@ Options
Enable or disable the analysis of function parameters, like
``void foo(int* ptr)``. Only reference and pointer parameters are analyzed.
Unnamed parameters, parameters of ``main``, member functions (including
- constructors) and lambdas are excluded from the analysis. Default is `true`.
+ constructors) and lambdas are excluded from the analysis. If a function is
+ used other than by calling it, e.g. its address is taken, ``const`` is not
+ suggested for the pointee or reference of its parameters, as that would
+ change the function type. Only uses in the current translation unit are
+ seen. Default is `true`.
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
index cbe63d878ccb60..afa53d7c10a4ad 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
@@ -545,3 +545,59 @@ void named_in_decl_unnamed_in_def(int&) {
// If the parameter is unnamed in the definition, it will not be reported, even
// if it is named in some declaration.
}
+
+// Adding 'const' to the pointee or reference would change the function type,
+// which breaks code that uses the function as a value.
+void destroy(Bar* const p) { delete p; }
+void (*destroy_ptr)(Bar*) = &destroy;
+
+void take_callback(void (*)(Bar*));
+
+void decayed_to_pointer(Bar* b) {
+ // CHECK-MESSAGES: [[@LINE-1]]:25: warning: variable 'b' of type 'Bar *' can be declared 'const'
+ // CHECK-FIXES: void decayed_to_pointer(Bar* const b) {
+ b->const_method();
+}
+
+void use_decayed_to_pointer() {
+ take_callback(decayed_to_pointer);
+}
+
+void bound_to_reference(Bar& b) {
+ b.const_method();
+}
+void (&bound_ref)(Bar&) = bound_to_reference;
+
+void template_argument(Bar& b) {
+ b.const_method();
+}
+template <void (*)(Bar&)> struct TakesFunction {};
+TakesFunction<&template_argument> takes_function;
+
+void address_taken_before_definition(Bar& b);
+void (*early_ptr)(Bar&) = &address_taken_before_definition;
+void address_taken_before_definition(Bar& b) {
+ b.const_method();
+}
+
+void only_called(Bar& b) {
+ // CHECK-MESSAGES: [[@LINE-1]]:18: warning: variable 'b' of type 'Bar &' can be declared 'const'
+ // CHECK-FIXES: void only_called(Bar const& b) {
+ b.const_method();
+}
+
+void call_only_called(Bar& b) {
+ only_called(b);
+ (only_called)(b);
+}
+
+Bar operator+(Bar& l, Bar& r) {
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: variable 'l' of type 'Bar &' can be declared 'const'
+ // CHECK-MESSAGES: [[@LINE-2]]:23: warning: variable 'r' of type 'Bar &' can be declared 'const'
+ // CHECK-FIXES: Bar operator+(Bar const& l, Bar const& r) {
+ return l;
+}
+
+void call_operator(Bar& l, Bar& r) {
+ l + r;
+}
|
|
@llvm/pr-subscribers-clang-tools-extra Author: Yash Aggarwal (yashaggarwal85d) Changesmisc-const-correctness suggests Now, if a function is referenced anywhere in the TU other than as the callee Uses in other TUs still can't be seen. It's also a bit conservative: a Fixes #236012 5 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
index 4532a27962af29..02a11549dc0ab3 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
@@ -13,6 +13,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include <cassert>
using namespace clang::ast_matchers;
@@ -297,6 +298,10 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
// Offload const-analysis to utility function.
if (isMutated(Variable, LocalScope, Function, Result.Context))
return;
+ // Adding 'const' to a reference parameter changes the function type.
+ if (VC == VariableCategory::Reference && isa<ParmVarDecl>(Variable) &&
+ isUsedAsValue(Function, *Result.Context))
+ return;
const auto Diag = diag(Variable->getBeginLoc(),
"variable %0 of type %1 can be declared 'const'")
@@ -336,6 +341,8 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
registerScope(LocalScope, Result.Context);
if (ScopesCache[LocalScope]->isPointeeMutated(Variable))
return;
+ if (isa<ParmVarDecl>(Variable) && isUsedAsValue(Function, *Result.Context))
+ return;
const auto Diag =
diag(Variable->getBeginLoc(),
"pointee of variable %0 of type %1 can be declared 'const'")
@@ -383,6 +390,28 @@ void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
}
}
+bool ConstCorrectnessCheck::isUsedAsValue(const FunctionDecl *Func,
+ ASTContext &Context) {
+ if (!FunctionsUsedAsValue) {
+ FunctionsUsedAsValue.emplace();
+ const auto Matches =
+ match(translationUnitDecl(forEachDescendant(
+ stmt(eachOf(declRefExpr(to(functionDecl())).bind("ref"),
+ callExpr(callee(expr(ignoringParenImpCasts(
+ declRefExpr().bind("callee"))))))))),
+ Context);
+ llvm::SmallPtrSet<const DeclRefExpr *, 32> Callees;
+ for (const auto &Match : Matches)
+ if (const auto *Callee = Match.getNodeAs<DeclRefExpr>("callee"))
+ Callees.insert(Callee);
+ for (const auto &Match : Matches)
+ if (const auto *Ref = Match.getNodeAs<DeclRefExpr>("ref");
+ Ref && !Callees.contains(Ref))
+ FunctionsUsedAsValue->insert(Ref->getDecl()->getCanonicalDecl());
+ }
+ return FunctionsUsedAsValue->contains(Func->getCanonicalDecl());
+}
+
void ConstCorrectnessCheck::registerScope(const Stmt *LocalScope,
ASTContext *Context) {
auto &Analyzer = ScopesCache[LocalScope];
diff --git a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
index 2e0e2fc67564b3..f996d7e57bf9e5 100644
--- a/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
@@ -12,6 +12,7 @@
#include "../ClangTidyCheck.h"
#include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
#include "llvm/ADT/DenseSet.h"
+#include <optional>
namespace clang::tidy::misc {
@@ -30,12 +31,18 @@ class ConstCorrectnessCheck : public ClangTidyCheck {
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override { FunctionsUsedAsValue.reset(); }
private:
void registerScope(const Stmt *LocalScope, ASTContext *Context);
bool isMutated(const VarDecl *Variable, const Stmt *Scope,
const FunctionDecl *Func, ASTContext *Context);
+ // Whether the function is referenced other than by being called, e.g. its
+ // address is taken. Its parameter types can't be changed then.
+ bool isUsedAsValue(const FunctionDecl *Func, ASTContext &Context);
+ std::optional<llvm::DenseSet<const Decl *>> FunctionsUsedAsValue;
+
using MutationAnalyzer = std::unique_ptr<ExprMutationAnalyzer>;
llvm::DenseMap<const Stmt *, MutationAnalyzer> ScopesCache;
llvm::DenseSet<SourceLocation> TemplateDiagnosticsCache;
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 310b084c04eb04..02a84a9d3e8be7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -274,6 +274,10 @@ infrastructure are described first, followed by tool-specific sections.
instantiations, where the suggested `const` would break the move for
instantiations with real move semantics.
+ - No longer suggests adding `const` to the pointee or reference of a
+ parameter when the function is used as a value, such as having its
+ address taken, where the changed signature would no longer match.
+
- Fixed an infinite loop in {doc}`misc-multiple-inheritance
<clang-tidy/checks/misc/multiple-inheritance>` when checking a class that
inherits from itself or has a circular inheritance graph.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
index 0a1eaf0e45a57d..d5854db7d8110a 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -123,7 +123,11 @@ Options
Enable or disable the analysis of function parameters, like
``void foo(int* ptr)``. Only reference and pointer parameters are analyzed.
Unnamed parameters, parameters of ``main``, member functions (including
- constructors) and lambdas are excluded from the analysis. Default is `true`.
+ constructors) and lambdas are excluded from the analysis. If a function is
+ used other than by calling it, e.g. its address is taken, ``const`` is not
+ suggested for the pointee or reference of its parameters, as that would
+ change the function type. Only uses in the current translation unit are
+ seen. Default is `true`.
.. code-block:: c++
diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
index cbe63d878ccb60..afa53d7c10a4ad 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-parameters.cpp
@@ -545,3 +545,59 @@ void named_in_decl_unnamed_in_def(int&) {
// If the parameter is unnamed in the definition, it will not be reported, even
// if it is named in some declaration.
}
+
+// Adding 'const' to the pointee or reference would change the function type,
+// which breaks code that uses the function as a value.
+void destroy(Bar* const p) { delete p; }
+void (*destroy_ptr)(Bar*) = &destroy;
+
+void take_callback(void (*)(Bar*));
+
+void decayed_to_pointer(Bar* b) {
+ // CHECK-MESSAGES: [[@LINE-1]]:25: warning: variable 'b' of type 'Bar *' can be declared 'const'
+ // CHECK-FIXES: void decayed_to_pointer(Bar* const b) {
+ b->const_method();
+}
+
+void use_decayed_to_pointer() {
+ take_callback(decayed_to_pointer);
+}
+
+void bound_to_reference(Bar& b) {
+ b.const_method();
+}
+void (&bound_ref)(Bar&) = bound_to_reference;
+
+void template_argument(Bar& b) {
+ b.const_method();
+}
+template <void (*)(Bar&)> struct TakesFunction {};
+TakesFunction<&template_argument> takes_function;
+
+void address_taken_before_definition(Bar& b);
+void (*early_ptr)(Bar&) = &address_taken_before_definition;
+void address_taken_before_definition(Bar& b) {
+ b.const_method();
+}
+
+void only_called(Bar& b) {
+ // CHECK-MESSAGES: [[@LINE-1]]:18: warning: variable 'b' of type 'Bar &' can be declared 'const'
+ // CHECK-FIXES: void only_called(Bar const& b) {
+ b.const_method();
+}
+
+void call_only_called(Bar& b) {
+ only_called(b);
+ (only_called)(b);
+}
+
+Bar operator+(Bar& l, Bar& r) {
+ // CHECK-MESSAGES: [[@LINE-1]]:15: warning: variable 'l' of type 'Bar &' can be declared 'const'
+ // CHECK-MESSAGES: [[@LINE-2]]:23: warning: variable 'r' of type 'Bar &' can be declared 'const'
+ // CHECK-FIXES: Bar operator+(Bar const& l, Bar const& r) {
+ return l;
+}
+
+void call_operator(Bar& l, Bar& r) {
+ l + r;
+}
|
Contributor
Author
|
Read the policies, thanks |
This branch has not been deployed
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
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.
misc-const-correctness suggests
constfor the pointee or reference of aparameter by looking only at the function body. If the function is also used
as a value, e.g.
&destroypassed as aunique_ptrdeleter, applying the fixchanges the function type and the code stops compiling.
Now, if a function is referenced anywhere in the TU other than as the callee
of a call (address taken, decays to a pointer as an argument, bound to a
function reference, used as a template argument), the check skips the pointee
and reference suggestions for its parameters. The top-level
T *constsuggestion still fires because it doesn't change the type. The set of such
functions is built once per TU, and only when a parameter diagnostic is about
to be emitted.
Uses in other TUs still can't be seen. It's also a bit conservative: a
function passed to something like
std::sortas a comparator no longer getsthe suggestion, even though
constwould compile there.Fixes #236012
This was done with AI assistance; I reviewed and tested it myself.