[NFC] Complete proper copying and resource cleanup in classes. #118655
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Member
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clangd Author: Zahira Ammarguellat (zahiraam) ChangesProvide, where missing, a copy constructor, a copy assignment operator or a destructor to prevent potential issues that can arise. Full diff: https://github.com/llvm/llvm-project/pull/118655.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index 97e16a12febd04..d9efe42735f2fc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -81,6 +81,9 @@ class ClangTidyContext {
~ClangTidyContext();
+ ClangTidyContext(const ClangTidyContext &) = default;
+ ClangTidyContext &operator=(const ClangTidyContext &) = default;
+
/// Report any errors detected using this method.
///
/// This is still under heavy development and will likely change towards using
diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
index e862195abaabbe..1e7485224de116 100644
--- a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
+++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
@@ -31,6 +31,8 @@ class NoLintDirectiveHandler {
public:
NoLintDirectiveHandler();
~NoLintDirectiveHandler();
+ NoLintDirectiveHandler(const NoLintDirectiveHandler &) = default;
+ NoLintDirectiveHandler &operator=(const NoLintDirectiveHandler &) = default;
bool shouldSuppress(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Diag, llvm::StringRef DiagName,
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 0b8e4720f53236..607478aa821c87 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -73,6 +73,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
/// The destructor blocks on any outstanding background tasks.
~ClangdLSPServer();
+ ClangdLSPServer(const ClangdLSPServer &other) = default;
+ ClangdLSPServer &operator=(const ClangdLSPServer &other) = default;
+
/// Run LSP server loop, communicating with the Transport provided in the
/// constructor. This method must not be executed more than once.
///
diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h
index 63e564bd68a78c..b28cf927e4c0c6 100644
--- a/clang-tools-extra/clangd/ParsedAST.h
+++ b/clang-tools-extra/clangd/ParsedAST.h
@@ -59,6 +59,9 @@ class ParsedAST {
~ParsedAST();
+ ParsedAST(const ParsedAST &Other) = default;
+ ParsedAST &operator=(const ParsedAST &Other) = default;
+
/// Note that the returned ast will not contain decls from the preamble that
/// were not deserialized during parsing. Clients should expect only decls
/// from the main file to be in the AST.
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 71548b59cc3088..651e03eced7fb9 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -411,6 +411,9 @@ class PreambleThrottlerRequest {
if (Throttler)
Throttler->release(ID);
}
+ PreambleThrottlerRequest(const PreambleThrottlerRequest &) = default;
+ PreambleThrottlerRequest &
+ operator=(const PreambleThrottlerRequest &) = default;
private:
PreambleThrottler::RequestID ID;
@@ -621,7 +624,8 @@ class ASTWorker {
AsyncTaskRunner *Tasks, Semaphore &Barrier,
const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks);
~ASTWorker();
-
+ ASTWorker(const ASTWorker &other) = default;
+ ASTWorker &operator=(const ASTWorker &other) = default;
void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged);
void
runWithAST(llvm::StringRef Name,
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index fb936d46bbcf7e..70848d2e053413 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -242,6 +242,9 @@ class TUScheduler {
std::unique_ptr<ParsingCallbacks> ASTCallbacks = nullptr);
~TUScheduler();
+ TUScheduler(const TUScheduler &other) = default;
+ TUScheduler &operator=(const TUScheduler &other) = default;
+
struct FileStats {
std::size_t UsedBytesAST = 0;
std::size_t UsedBytesPreamble = 0;
diff --git a/clang-tools-extra/clangd/support/DirectiveTree.cpp b/clang-tools-extra/clangd/support/DirectiveTree.cpp
index d25da111681afc..268dde45196493 100644
--- a/clang-tools-extra/clangd/support/DirectiveTree.cpp
+++ b/clang-tools-extra/clangd/support/DirectiveTree.cpp
@@ -328,6 +328,9 @@ class Preprocessor {
Preprocessor(const TokenStream &In, TokenStream &Out) : In(In), Out(Out) {}
~Preprocessor() { Out.finalize(); }
+ Preprocessor(const Preprocessor &other) = default;
+ Preprocessor &operator=(const Preprocessor &other) = default;
+
void walk(const DirectiveTree &T) {
for (const auto &C : T.Chunks)
std::visit(*this, C);
diff --git a/clang-tools-extra/modularize/ModuleAssistant.cpp b/clang-tools-extra/modularize/ModuleAssistant.cpp
index 5c11ffdb8589d5..b6bb1d5146e560 100644
--- a/clang-tools-extra/modularize/ModuleAssistant.cpp
+++ b/clang-tools-extra/modularize/ModuleAssistant.cpp
@@ -46,6 +46,8 @@ class Module {
public:
Module(llvm::StringRef Name, bool Problem);
~Module();
+ Module(const Module &other) = default;
+ Module &operator=(const Module &other) = default;
bool output(llvm::raw_fd_ostream &OS, int Indent);
Module *findSubModule(llvm::StringRef SubName);
|
Member
|
@llvm/pr-subscribers-clang-tidy Author: Zahira Ammarguellat (zahiraam) ChangesProvide, where missing, a copy constructor, a copy assignment operator or a destructor to prevent potential issues that can arise. Full diff: https://github.com/llvm/llvm-project/pull/118655.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index 97e16a12febd04..d9efe42735f2fc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -81,6 +81,9 @@ class ClangTidyContext {
~ClangTidyContext();
+ ClangTidyContext(const ClangTidyContext &) = default;
+ ClangTidyContext &operator=(const ClangTidyContext &) = default;
+
/// Report any errors detected using this method.
///
/// This is still under heavy development and will likely change towards using
diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
index e862195abaabbe..1e7485224de116 100644
--- a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
+++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
@@ -31,6 +31,8 @@ class NoLintDirectiveHandler {
public:
NoLintDirectiveHandler();
~NoLintDirectiveHandler();
+ NoLintDirectiveHandler(const NoLintDirectiveHandler &) = default;
+ NoLintDirectiveHandler &operator=(const NoLintDirectiveHandler &) = default;
bool shouldSuppress(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Diag, llvm::StringRef DiagName,
diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h
index 0b8e4720f53236..607478aa821c87 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.h
+++ b/clang-tools-extra/clangd/ClangdLSPServer.h
@@ -73,6 +73,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks,
/// The destructor blocks on any outstanding background tasks.
~ClangdLSPServer();
+ ClangdLSPServer(const ClangdLSPServer &other) = default;
+ ClangdLSPServer &operator=(const ClangdLSPServer &other) = default;
+
/// Run LSP server loop, communicating with the Transport provided in the
/// constructor. This method must not be executed more than once.
///
diff --git a/clang-tools-extra/clangd/ParsedAST.h b/clang-tools-extra/clangd/ParsedAST.h
index 63e564bd68a78c..b28cf927e4c0c6 100644
--- a/clang-tools-extra/clangd/ParsedAST.h
+++ b/clang-tools-extra/clangd/ParsedAST.h
@@ -59,6 +59,9 @@ class ParsedAST {
~ParsedAST();
+ ParsedAST(const ParsedAST &Other) = default;
+ ParsedAST &operator=(const ParsedAST &Other) = default;
+
/// Note that the returned ast will not contain decls from the preamble that
/// were not deserialized during parsing. Clients should expect only decls
/// from the main file to be in the AST.
diff --git a/clang-tools-extra/clangd/TUScheduler.cpp b/clang-tools-extra/clangd/TUScheduler.cpp
index 71548b59cc3088..651e03eced7fb9 100644
--- a/clang-tools-extra/clangd/TUScheduler.cpp
+++ b/clang-tools-extra/clangd/TUScheduler.cpp
@@ -411,6 +411,9 @@ class PreambleThrottlerRequest {
if (Throttler)
Throttler->release(ID);
}
+ PreambleThrottlerRequest(const PreambleThrottlerRequest &) = default;
+ PreambleThrottlerRequest &
+ operator=(const PreambleThrottlerRequest &) = default;
private:
PreambleThrottler::RequestID ID;
@@ -621,7 +624,8 @@ class ASTWorker {
AsyncTaskRunner *Tasks, Semaphore &Barrier,
const TUScheduler::Options &Opts, ParsingCallbacks &Callbacks);
~ASTWorker();
-
+ ASTWorker(const ASTWorker &other) = default;
+ ASTWorker &operator=(const ASTWorker &other) = default;
void update(ParseInputs Inputs, WantDiagnostics, bool ContentChanged);
void
runWithAST(llvm::StringRef Name,
diff --git a/clang-tools-extra/clangd/TUScheduler.h b/clang-tools-extra/clangd/TUScheduler.h
index fb936d46bbcf7e..70848d2e053413 100644
--- a/clang-tools-extra/clangd/TUScheduler.h
+++ b/clang-tools-extra/clangd/TUScheduler.h
@@ -242,6 +242,9 @@ class TUScheduler {
std::unique_ptr<ParsingCallbacks> ASTCallbacks = nullptr);
~TUScheduler();
+ TUScheduler(const TUScheduler &other) = default;
+ TUScheduler &operator=(const TUScheduler &other) = default;
+
struct FileStats {
std::size_t UsedBytesAST = 0;
std::size_t UsedBytesPreamble = 0;
diff --git a/clang-tools-extra/clangd/support/DirectiveTree.cpp b/clang-tools-extra/clangd/support/DirectiveTree.cpp
index d25da111681afc..268dde45196493 100644
--- a/clang-tools-extra/clangd/support/DirectiveTree.cpp
+++ b/clang-tools-extra/clangd/support/DirectiveTree.cpp
@@ -328,6 +328,9 @@ class Preprocessor {
Preprocessor(const TokenStream &In, TokenStream &Out) : In(In), Out(Out) {}
~Preprocessor() { Out.finalize(); }
+ Preprocessor(const Preprocessor &other) = default;
+ Preprocessor &operator=(const Preprocessor &other) = default;
+
void walk(const DirectiveTree &T) {
for (const auto &C : T.Chunks)
std::visit(*this, C);
diff --git a/clang-tools-extra/modularize/ModuleAssistant.cpp b/clang-tools-extra/modularize/ModuleAssistant.cpp
index 5c11ffdb8589d5..b6bb1d5146e560 100644
--- a/clang-tools-extra/modularize/ModuleAssistant.cpp
+++ b/clang-tools-extra/modularize/ModuleAssistant.cpp
@@ -46,6 +46,8 @@ class Module {
public:
Module(llvm::StringRef Name, bool Problem);
~Module();
+ Module(const Module &other) = default;
+ Module &operator=(const Module &other) = default;
bool output(llvm::raw_fd_ostream &OS, int Indent);
Module *findSubModule(llvm::StringRef SubName);
|
HerrCai0907
reviewed
Dec 4, 2024
Contributor
HerrCai0907
left a comment
HerrCai0907
left a comment
There was a problem hiding this comment.
I don't think we will have potential issue if we just miss this default copy construct and copy assignment. Could you explain more about the reason?
Contributor
Author
This was an issue revealed by a static analyzer. |
AaronBallman
reviewed
Dec 5, 2024
Collaborator
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
I don't think any of these changes are strictly necessary (they're not fixing any bugs), but I do think they may be somewhat reasonable as an NFC cleanup if we're explicitly marking functions as deleted because we don't intend for the type to be used as a value type. WDYT @HerrCai0907 ?
Comment on lines
84
to
85
| ClangTidyContext(const ClangTidyContext &) = default; | ||
| ClangTidyContext &operator=(const ClangTidyContext &) = default; |
Collaborator
There was a problem hiding this comment.
I think these would make more sense as = delete; so that we prevent accidental copies of the context object. (We want this passed around by reference or pointer only.)
Comment on lines
34
to
35
| NoLintDirectiveHandler(const NoLintDirectiveHandler &) = default; | ||
| NoLintDirectiveHandler &operator=(const NoLintDirectiveHandler &) = default; |
Collaborator
There was a problem hiding this comment.
I don't think these are strictly needed, but I think it would be fine to explicitly = delete them.
Contributor
Author
|
Thanks @AaronBallman . I think it makes more sense to have |
Contributor
|
Agree, mark delete definitely make sense. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Provide, where missing, a copy constructor, a copy assignment operator or a destructor to prevent potential issues that can arise.