[clang][NFCI] Clarify ownership of PragmaHandlers #117703
Closed
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
Initially found by static analysis tool, the situation with the same memory "owned" by two different uniqie_ptrs (one in Parser, another one in a PragmaNamespace that is owned by Preprocessor) DOES seem like a questionable design. This patch switches to shared_ptr to reflect this shared ownership better.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Mariya Podchishchaeva (Fznamznon) ChangesInitially found by static analysis tool, the situation with the same memory "owned" by two different uniqie_ptrs (one in Parser, another one in a PragmaNamespace that is owned by Preprocessor) DOES seem like a questionable design. This patch switches to shared_ptr to reflect this shared ownership better. Patch is 40.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117703.diff 6 Files Affected:
diff --git a/clang/include/clang/Lex/Pragma.h b/clang/include/clang/Lex/Pragma.h
index 67eca618f6c4ff..69ad168c8115fa 100644
--- a/clang/include/clang/Lex/Pragma.h
+++ b/clang/include/clang/Lex/Pragma.h
@@ -96,7 +96,7 @@ class EmptyPragmaHandler : public PragmaHandler {
class PragmaNamespace : public PragmaHandler {
/// Handlers - This is a map of the handlers in this namespace with their name
/// as key.
- llvm::StringMap<std::unique_ptr<PragmaHandler>> Handlers;
+ llvm::StringMap<std::shared_ptr<PragmaHandler>> Handlers;
public:
explicit PragmaNamespace(StringRef Name) : PragmaHandler(Name) {}
@@ -109,11 +109,11 @@ class PragmaNamespace : public PragmaHandler {
bool IgnoreNull = true) const;
/// AddPragma - Add a pragma to this namespace.
- void AddPragma(PragmaHandler *Handler);
+ void AddPragma(std::shared_ptr<PragmaHandler> Handler);
/// RemovePragmaHandler - Remove the given handler from the
/// namespace.
- void RemovePragmaHandler(PragmaHandler *Handler);
+ void RemovePragmaHandler(StringRef Name);
bool IsEmpty() const { return Handlers.empty(); }
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 3312d4ed1d798d..168f46d0adf2f2 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -1530,19 +1530,20 @@ class Preprocessor {
///
/// If \p Namespace is non-null, then it is a token required to exist on the
/// pragma line before the pragma string starts, e.g. "STDC" or "GCC".
- void AddPragmaHandler(StringRef Namespace, PragmaHandler *Handler);
- void AddPragmaHandler(PragmaHandler *Handler) {
+ void AddPragmaHandler(StringRef Namespace,
+ std::shared_ptr<PragmaHandler> Handler);
+ void AddPragmaHandler(std::shared_ptr<PragmaHandler> Handler) {
AddPragmaHandler(StringRef(), Handler);
}
/// Remove the specific pragma handler from this preprocessor.
///
/// If \p Namespace is non-null, then it should be the namespace that
- /// \p Handler was added to. It is an error to remove a handler that
+ /// \p HandlerName was added to. It is an error to remove a handler that
/// has not been registered.
- void RemovePragmaHandler(StringRef Namespace, PragmaHandler *Handler);
- void RemovePragmaHandler(PragmaHandler *Handler) {
- RemovePragmaHandler(StringRef(), Handler);
+ void RemovePragmaHandler(StringRef Namespace, StringRef HandlerName);
+ void RemovePragmaHandler(StringRef HandlerName) {
+ RemovePragmaHandler(StringRef(), HandlerName);
}
/// Install empty handlers for all pragmas (making them ignored).
diff --git a/clang/include/clang/Parse/Parser.h b/clang/include/clang/Parse/Parser.h
index d3838a4cc8418c..032b0f7cc4669f 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -173,54 +173,57 @@ class Parser : public CodeCompletionHandler {
// used as type traits.
llvm::SmallDenseMap<IdentifierInfo *, tok::TokenKind> RevertibleTypeTraits;
- std::unique_ptr<PragmaHandler> AlignHandler;
- std::unique_ptr<PragmaHandler> GCCVisibilityHandler;
- std::unique_ptr<PragmaHandler> OptionsHandler;
- std::unique_ptr<PragmaHandler> PackHandler;
- std::unique_ptr<PragmaHandler> MSStructHandler;
- std::unique_ptr<PragmaHandler> UnusedHandler;
- std::unique_ptr<PragmaHandler> WeakHandler;
- std::unique_ptr<PragmaHandler> RedefineExtnameHandler;
- std::unique_ptr<PragmaHandler> FPContractHandler;
- std::unique_ptr<PragmaHandler> OpenCLExtensionHandler;
- std::unique_ptr<PragmaHandler> OpenMPHandler;
- std::unique_ptr<PragmaHandler> OpenACCHandler;
- std::unique_ptr<PragmaHandler> PCSectionHandler;
- std::unique_ptr<PragmaHandler> MSCommentHandler;
- std::unique_ptr<PragmaHandler> MSDetectMismatchHandler;
- std::unique_ptr<PragmaHandler> FPEvalMethodHandler;
- std::unique_ptr<PragmaHandler> FloatControlHandler;
- std::unique_ptr<PragmaHandler> MSPointersToMembers;
- std::unique_ptr<PragmaHandler> MSVtorDisp;
- std::unique_ptr<PragmaHandler> MSInitSeg;
- std::unique_ptr<PragmaHandler> MSDataSeg;
- std::unique_ptr<PragmaHandler> MSBSSSeg;
- std::unique_ptr<PragmaHandler> MSConstSeg;
- std::unique_ptr<PragmaHandler> MSCodeSeg;
- std::unique_ptr<PragmaHandler> MSSection;
- std::unique_ptr<PragmaHandler> MSStrictGuardStackCheck;
- std::unique_ptr<PragmaHandler> MSRuntimeChecks;
- std::unique_ptr<PragmaHandler> MSIntrinsic;
- std::unique_ptr<PragmaHandler> MSFunction;
- std::unique_ptr<PragmaHandler> MSOptimize;
- std::unique_ptr<PragmaHandler> MSFenvAccess;
- std::unique_ptr<PragmaHandler> MSAllocText;
- std::unique_ptr<PragmaHandler> CUDAForceHostDeviceHandler;
- std::unique_ptr<PragmaHandler> OptimizeHandler;
- std::unique_ptr<PragmaHandler> LoopHintHandler;
- std::unique_ptr<PragmaHandler> UnrollHintHandler;
- std::unique_ptr<PragmaHandler> NoUnrollHintHandler;
- std::unique_ptr<PragmaHandler> UnrollAndJamHintHandler;
- std::unique_ptr<PragmaHandler> NoUnrollAndJamHintHandler;
- std::unique_ptr<PragmaHandler> FPHandler;
- std::unique_ptr<PragmaHandler> STDCFenvAccessHandler;
- std::unique_ptr<PragmaHandler> STDCFenvRoundHandler;
- std::unique_ptr<PragmaHandler> STDCCXLIMITHandler;
- std::unique_ptr<PragmaHandler> STDCUnknownHandler;
- std::unique_ptr<PragmaHandler> AttributePragmaHandler;
- std::unique_ptr<PragmaHandler> MaxTokensHerePragmaHandler;
- std::unique_ptr<PragmaHandler> MaxTokensTotalPragmaHandler;
- std::unique_ptr<PragmaHandler> RISCVPragmaHandler;
+ /// Factory object for creating ParsedAttr objects.
+ AttributeFactory AttrFactory;
+
+ std::shared_ptr<PragmaHandler> AlignHandler;
+ std::shared_ptr<PragmaHandler> GCCVisibilityHandler;
+ std::shared_ptr<PragmaHandler> OptionsHandler;
+ std::shared_ptr<PragmaHandler> PackHandler;
+ std::shared_ptr<PragmaHandler> MSStructHandler;
+ std::shared_ptr<PragmaHandler> UnusedHandler;
+ std::shared_ptr<PragmaHandler> WeakHandler;
+ std::shared_ptr<PragmaHandler> RedefineExtnameHandler;
+ std::shared_ptr<PragmaHandler> FPContractHandler;
+ std::shared_ptr<PragmaHandler> OpenCLExtensionHandler;
+ std::shared_ptr<PragmaHandler> OpenMPHandler;
+ std::shared_ptr<PragmaHandler> OpenACCHandler;
+ std::shared_ptr<PragmaHandler> PCSectionHandler;
+ std::shared_ptr<PragmaHandler> MSCommentHandler;
+ std::shared_ptr<PragmaHandler> MSDetectMismatchHandler;
+ std::shared_ptr<PragmaHandler> FPEvalMethodHandler;
+ std::shared_ptr<PragmaHandler> FloatControlHandler;
+ std::shared_ptr<PragmaHandler> MSPointersToMembers;
+ std::shared_ptr<PragmaHandler> MSVtorDisp;
+ std::shared_ptr<PragmaHandler> MSInitSeg;
+ std::shared_ptr<PragmaHandler> MSDataSeg;
+ std::shared_ptr<PragmaHandler> MSBSSSeg;
+ std::shared_ptr<PragmaHandler> MSConstSeg;
+ std::shared_ptr<PragmaHandler> MSCodeSeg;
+ std::shared_ptr<PragmaHandler> MSSection;
+ std::shared_ptr<PragmaHandler> MSStrictGuardStackCheck;
+ std::shared_ptr<PragmaHandler> MSRuntimeChecks;
+ std::shared_ptr<PragmaHandler> MSIntrinsic;
+ std::shared_ptr<PragmaHandler> MSFunction;
+ std::shared_ptr<PragmaHandler> MSOptimize;
+ std::shared_ptr<PragmaHandler> MSFenvAccess;
+ std::shared_ptr<PragmaHandler> MSAllocText;
+ std::shared_ptr<PragmaHandler> CUDAForceHostDeviceHandler;
+ std::shared_ptr<PragmaHandler> OptimizeHandler;
+ std::shared_ptr<PragmaHandler> LoopHintHandler;
+ std::shared_ptr<PragmaHandler> UnrollHintHandler;
+ std::shared_ptr<PragmaHandler> NoUnrollHintHandler;
+ std::shared_ptr<PragmaHandler> UnrollAndJamHintHandler;
+ std::shared_ptr<PragmaHandler> NoUnrollAndJamHintHandler;
+ std::shared_ptr<PragmaHandler> FPHandler;
+ std::shared_ptr<PragmaHandler> STDCFenvAccessHandler;
+ std::shared_ptr<PragmaHandler> STDCFenvRoundHandler;
+ std::shared_ptr<PragmaHandler> STDCCXLIMITHandler;
+ std::shared_ptr<PragmaHandler> STDCUnknownHandler;
+ std::shared_ptr<PragmaHandler> AttributePragmaHandler;
+ std::shared_ptr<PragmaHandler> MaxTokensHerePragmaHandler;
+ std::shared_ptr<PragmaHandler> MaxTokensTotalPragmaHandler;
+ std::shared_ptr<PragmaHandler> RISCVPragmaHandler;
std::unique_ptr<CommentHandler> CommentSemaHandler;
@@ -311,9 +314,6 @@ class Parser : public CodeCompletionHandler {
unsigned getOriginalDepth() const { return Depth - AddedLevels; }
};
- /// Factory object for creating ParsedAttr objects.
- AttributeFactory AttrFactory;
-
/// Gathers and cleans up TemplateIdAnnotations when parsing of a
/// top-level declaration is finished.
SmallVector<TemplateIdAnnotation *, 16> TemplateIds;
diff --git a/clang/lib/Frontend/PrintPreprocessedOutput.cpp b/clang/lib/Frontend/PrintPreprocessedOutput.cpp
index 1005825441b3e6..7686c7acffcdee 100644
--- a/clang/lib/Frontend/PrintPreprocessedOutput.cpp
+++ b/clang/lib/Frontend/PrintPreprocessedOutput.cpp
@@ -1095,32 +1095,32 @@ void clang::DoPrintPreprocessedInput(Preprocessor &PP, raw_ostream *OS,
// Expand macros in pragmas with -fms-extensions. The assumption is that
// the majority of pragmas in such a file will be Microsoft pragmas.
// Remember the handlers we will add so that we can remove them later.
- std::unique_ptr<UnknownPragmaHandler> MicrosoftExtHandler(
+ std::shared_ptr<UnknownPragmaHandler> MicrosoftExtHandler(
new UnknownPragmaHandler(
"#pragma", Callbacks,
/*RequireTokenExpansion=*/PP.getLangOpts().MicrosoftExt));
- std::unique_ptr<UnknownPragmaHandler> GCCHandler(new UnknownPragmaHandler(
+ std::shared_ptr<UnknownPragmaHandler> GCCHandler(new UnknownPragmaHandler(
"#pragma GCC", Callbacks,
/*RequireTokenExpansion=*/PP.getLangOpts().MicrosoftExt));
- std::unique_ptr<UnknownPragmaHandler> ClangHandler(new UnknownPragmaHandler(
+ std::shared_ptr<UnknownPragmaHandler> ClangHandler(new UnknownPragmaHandler(
"#pragma clang", Callbacks,
/*RequireTokenExpansion=*/PP.getLangOpts().MicrosoftExt));
- PP.AddPragmaHandler(MicrosoftExtHandler.get());
- PP.AddPragmaHandler("GCC", GCCHandler.get());
- PP.AddPragmaHandler("clang", ClangHandler.get());
+ PP.AddPragmaHandler(MicrosoftExtHandler);
+ PP.AddPragmaHandler("GCC", GCCHandler);
+ PP.AddPragmaHandler("clang", ClangHandler);
// The tokens after pragma omp need to be expanded.
//
// OpenMP [2.1, Directive format]
// Preprocessing tokens following the #pragma omp are subject to macro
// replacement.
- std::unique_ptr<UnknownPragmaHandler> OpenMPHandler(
+ std::shared_ptr<UnknownPragmaHandler> OpenMPHandler(
new UnknownPragmaHandler("#pragma omp", Callbacks,
/*RequireTokenExpansion=*/true));
- PP.AddPragmaHandler("omp", OpenMPHandler.get());
+ PP.AddPragmaHandler("omp", OpenMPHandler);
PP.addPPCallbacks(std::unique_ptr<PPCallbacks>(Callbacks));
@@ -1153,8 +1153,8 @@ void clang::DoPrintPreprocessedInput(Preprocessor &PP, raw_ostream *OS,
// Remove the handlers we just added to leave the preprocessor in a sane state
// so that it can be reused (for example by a clang::Parser instance).
- PP.RemovePragmaHandler(MicrosoftExtHandler.get());
- PP.RemovePragmaHandler("GCC", GCCHandler.get());
- PP.RemovePragmaHandler("clang", ClangHandler.get());
- PP.RemovePragmaHandler("omp", OpenMPHandler.get());
+ PP.RemovePragmaHandler(MicrosoftExtHandler->getName());
+ PP.RemovePragmaHandler("GCC", GCCHandler->getName());
+ PP.RemovePragmaHandler("clang", ClangHandler->getName());
+ PP.RemovePragmaHandler("omp", OpenMPHandler->getName());
}
diff --git a/clang/lib/Lex/Pragma.cpp b/clang/lib/Lex/Pragma.cpp
index e339ca84222784..8b385d8a9668b9 100644
--- a/clang/lib/Lex/Pragma.cpp
+++ b/clang/lib/Lex/Pragma.cpp
@@ -85,18 +85,15 @@ PragmaHandler *PragmaNamespace::FindHandler(StringRef Name,
return nullptr;
}
-void PragmaNamespace::AddPragma(PragmaHandler *Handler) {
+void PragmaNamespace::AddPragma(std::shared_ptr<PragmaHandler> Handler) {
assert(!Handlers.count(Handler->getName()) &&
"A handler with this name is already registered in this namespace");
- Handlers[Handler->getName()].reset(Handler);
+ Handlers[Handler->getName()] = Handler;
}
-void PragmaNamespace::RemovePragmaHandler(PragmaHandler *Handler) {
- auto I = Handlers.find(Handler->getName());
- assert(I != Handlers.end() &&
- "Handler not registered in this namespace");
- // Release ownership back to the caller.
- I->getValue().release();
+void PragmaNamespace::RemovePragmaHandler(StringRef Name) {
+ auto I = Handlers.find(Name);
+ assert(I != Handlers.end() && "Handler not registered in this namespace");
Handlers.erase(I);
}
@@ -909,7 +906,7 @@ void Preprocessor::HandlePragmaHdrstop(Token &Tok) {
/// If 'Namespace' is non-null, then it is a token required to exist on the
/// pragma line before the pragma string starts, e.g. "STDC" or "GCC".
void Preprocessor::AddPragmaHandler(StringRef Namespace,
- PragmaHandler *Handler) {
+ std::shared_ptr<PragmaHandler> Handler) {
PragmaNamespace *InsertNS = PragmaHandlers.get();
// If this is specified to be in a namespace, step down into it.
@@ -925,7 +922,7 @@ void Preprocessor::AddPragmaHandler(StringRef Namespace,
// Otherwise, this namespace doesn't exist yet, create and insert the
// handler for it.
InsertNS = new PragmaNamespace(Namespace);
- PragmaHandlers->AddPragma(InsertNS);
+ PragmaHandlers->AddPragma(std::shared_ptr<PragmaHandler>(InsertNS));
}
}
@@ -940,7 +937,7 @@ void Preprocessor::AddPragmaHandler(StringRef Namespace,
/// namespace that \arg Handler was added to. It is an error to remove
/// a handler that has not been registered.
void Preprocessor::RemovePragmaHandler(StringRef Namespace,
- PragmaHandler *Handler) {
+ StringRef HandlerName) {
PragmaNamespace *NS = PragmaHandlers.get();
// If this is specified to be in a namespace, step down into it.
@@ -952,13 +949,11 @@ void Preprocessor::RemovePragmaHandler(StringRef Namespace,
assert(NS && "Invalid namespace, registered as a regular pragma handler!");
}
- NS->RemovePragmaHandler(Handler);
+ NS->RemovePragmaHandler(HandlerName);
// If this is a non-default namespace and it is now empty, remove it.
- if (NS != PragmaHandlers.get() && NS->IsEmpty()) {
- PragmaHandlers->RemovePragmaHandler(NS);
- delete NS;
- }
+ if (NS != PragmaHandlers.get() && NS->IsEmpty())
+ PragmaHandlers->RemovePragmaHandler(NS->getName());
}
bool Preprocessor::LexOnOffSwitch(tok::OnOffSwitch &Result) {
@@ -2127,73 +2122,76 @@ struct PragmaFinalHandler : public PragmaHandler {
/// RegisterBuiltinPragmas - Install the standard preprocessor pragmas:
/// \#pragma GCC poison/system_header/dependency and \#pragma once.
void Preprocessor::RegisterBuiltinPragmas() {
- AddPragmaHandler(new PragmaOnceHandler());
- AddPragmaHandler(new PragmaMarkHandler());
- AddPragmaHandler(new PragmaPushMacroHandler());
- AddPragmaHandler(new PragmaPopMacroHandler());
- AddPragmaHandler(new PragmaMessageHandler(PPCallbacks::PMK_Message));
+ AddPragmaHandler(std::make_shared<PragmaOnceHandler>());
+ AddPragmaHandler(std::make_shared<PragmaMarkHandler>());
+ AddPragmaHandler(std::make_shared<PragmaPushMacroHandler>());
+ AddPragmaHandler(std::make_shared<PragmaPopMacroHandler>());
+ AddPragmaHandler(
+ std::make_shared<PragmaMessageHandler>(PPCallbacks::PMK_Message));
// #pragma GCC ...
- AddPragmaHandler("GCC", new PragmaPoisonHandler());
- AddPragmaHandler("GCC", new PragmaSystemHeaderHandler());
- AddPragmaHandler("GCC", new PragmaDependencyHandler());
- AddPragmaHandler("GCC", new PragmaDiagnosticHandler("GCC"));
- AddPragmaHandler("GCC", new PragmaMessageHandler(PPCallbacks::PMK_Warning,
- "GCC"));
- AddPragmaHandler("GCC", new PragmaMessageHandler(PPCallbacks::PMK_Error,
- "GCC"));
+ AddPragmaHandler("GCC", std::make_shared<PragmaPoisonHandler>());
+ AddPragmaHandler("GCC", std::make_shared<PragmaSystemHeaderHandler>());
+ AddPragmaHandler("GCC", std::make_shared<PragmaDependencyHandler>());
+ AddPragmaHandler("GCC", std::make_shared<PragmaDiagnosticHandler>("GCC"));
+ AddPragmaHandler("GCC", std::make_shared<PragmaMessageHandler>(
+ PPCallbacks::PMK_Warning, "GCC"));
+ AddPragmaHandler("GCC", std::make_shared<PragmaMessageHandler>(
+ PPCallbacks::PMK_Error, "GCC"));
// #pragma clang ...
- AddPragmaHandler("clang", new PragmaPoisonHandler());
- AddPragmaHandler("clang", new PragmaSystemHeaderHandler());
- AddPragmaHandler("clang", new PragmaDebugHandler());
- AddPragmaHandler("clang", new PragmaDependencyHandler());
- AddPragmaHandler("clang", new PragmaDiagnosticHandler("clang"));
- AddPragmaHandler("clang", new PragmaARCCFCodeAuditedHandler());
- AddPragmaHandler("clang", new PragmaAssumeNonNullHandler());
- AddPragmaHandler("clang", new PragmaDeprecatedHandler());
- AddPragmaHandler("clang", new PragmaRestrictExpansionHandler());
- AddPragmaHandler("clang", new PragmaFinalHandler());
+ AddPragmaHandler("clang", std::make_shared<PragmaPoisonHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaSystemHeaderHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaDebugHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaDependencyHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaDiagnosticHandler>("clang"));
+ AddPragmaHandler("clang", std::make_shared<PragmaARCCFCodeAuditedHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaAssumeNonNullHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaDeprecatedHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaRestrictExpansionHandler>());
+ AddPragmaHandler("clang", std::make_shared<PragmaFinalHandler>());
// #pragma clang module ...
- auto *ModuleHandler = new PragmaNamespace("module");
+ std::shared_ptr<PragmaNamespace> ModuleHandler =
+ std::make_shared<PragmaNamespace>("module");
AddPragmaHandler("clang", ModuleHandler);
- ModuleHandler->AddPragma(new PragmaModuleImportHandler());
- ModuleHandler->AddPragma(new PragmaModuleBeginHandler());
- ModuleHandler->AddPragma(new PragmaModuleEndHandler());
- ModuleHandler->AddPragma(new PragmaModuleBuildHandler());
- ModuleHandler->AddPragma(new PragmaModuleLoadHandler());
+ ModuleHandler->AddPragma(std::make_shared<PragmaModuleImportHandler>());
+ ModuleHandler->AddPragma(std::make_shared<PragmaModuleBeginHandler>());
+ ModuleHandler->AddPragma(std::make_shared<PragmaModuleEndHandler>());
+ ModuleHandler->AddPragma(std::make_shared<PragmaModuleBuildHandler>());
+ ModuleHandler->AddPragma(std::make_shared<PragmaModuleLoadHandler>());
// Safe Buffers pragmas
- AddPragmaHandler("clang", new PragmaUnsafeBufferUsageHandler);
+ AddPragmaHandler("clang", std::make_shared<PragmaUnsafeBufferUsageHandler>());
// Add region pragmas.
- AddPragmaHandler(new PragmaRegionHandler("region"));
- AddPragmaHandler(new PragmaRegionHandler("endregion"));
+ AddPragmaHandler(std::make_shared<PragmaRegionHandler>("region"));
+ AddPragmaHandler(std::make_shared<PragmaRegionHandler>("endregion"));
// MS extensions.
if (LangOpts.MicrosoftExt) {
- AddPragmaHandler(new PragmaWarningHandler());
- AddPragmaHandler(new PragmaExecCharsetHandler());
- AddPragmaHandler(new PragmaIncludeAliasHandler());
- AddPragmaHandler(new PragmaHdrstopHandler());
- AddPragmaHandler(new PragmaSystemHeaderHandler());
- AddPragmaHandler(new PragmaManagedHandler("managed"));
- AddPragmaHandler(new PragmaManagedHandler("unmanaged"));
+ AddPragmaHandler(std::make_shared<PragmaWarningHandler>());
+ AddPragmaHandler(std::make_shared<PragmaExecCharsetHandler>());
+ AddPragmaHandler(std::make_shared<PragmaIncludeAliasHandler>());
+ AddPragmaHandler(std::make_shared<PragmaHdrstopHandler>());
+ AddPragmaHandler(std::make_shared<PragmaSystemHeaderHandler>());
+ AddPragmaHandler(std::make_shared<PragmaManagedHandler>("managed"));
+ AddPragmaHandler(std::make_shared<PragmaManagedHandler>("unmanaged"));
}
// Pragmas added by plugins
for (const PragmaHandlerRegistry::e...
[truncated]
|
erichkeane
reviewed
Nov 26, 2024
Collaborator
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Unless I'm missing something, this doesn't really make sense. Pragma handlers being owned by the Parser makes perfect sense to me, and making them shared ownership does not make sense.
'Raw' pointers as non-owned references to a unique-ptr are completely sensible, so IMO, it is not a problem. Can you share what the Static assert complaint is that makes you think we should be doing this?
Contributor
Author
The complaint is that It seems I can't get rid of PragmaHandlers in Parser, since some of the handlers are "added" to Preprocessor under different namespaces using the same pointer owned by Parser (example UnrollHintHandler). It seems I can't remove a smart pointer from |
Collaborator
This is definitely concerning. But because we're properly managing lifetime, it seems sensible to me to just have
The problem with I would vastly prefer the destructor of the thing that needs to have |
Collaborator
|
I think the preprocessor is what owns the pragma handlers and the parser is what gets to observe them, right? Because the preprocessor is part of the clangLex library (and it needs to have pragma handlers to be able to process pp-tokens) and clangParse is built on top of clangLex. So I think that means the preprocessor is what should manage the memory for these and hold the unique_ptr, and the parser should just get a reference to the pragma handler so there's no lifetime to manage (because we know clangLex outlives clangParse anyway)? |
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.
Initially found by static analysis tool, the situation with the same memory "owned" by two different uniqie_ptrs (one in Parser, another one in a PragmaNamespace that is owned by Preprocessor) DOES seem like a questionable design. This patch switches to shared_ptr to reflect this shared ownership better.