[clang] Remove intrusive reference count from DiagnosticOptions
#139584
+603
−568
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-lldb @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThe void shareOwnership(CompilerInvocation &CI) {
llvm::IntrusiveRefCntPtr<DiagnosticOptions> CoOwner = &CI.getDiagnosticOptions();
// ...
}This is a perfectly valid pattern that is being actually used in the codebase. I would like to ensure the ownership of Patch is 189.21 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139584.diff 134 Files Affected:
diff --git a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
index 68b5743c6540f..062e236d3e51f 100644
--- a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -96,9 +96,9 @@ int main(int argc, char **argv) {
cl::SetVersionPrinter(printVersion);
cl::ParseCommandLineOptions(argc, argv);
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
+ DiagnosticOptions DiagOpts;
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts.get());
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts);
// Determine a formatting style from options.
auto FormatStyleOrError = format::getStyle(FormatStyleOpt, FormatStyleConfig,
diff --git a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
index 22d26db0c11bc..2a8fe2d06d185 100644
--- a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
+++ b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
@@ -126,10 +126,10 @@ int main(int argc, const char **argv) {
if (int Result = Tool.run(Factory.get()))
return Result;
LangOptions DefaultLangOptions;
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
- clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
SourceManager Sources(Diagnostics, FileMgr);
diff --git a/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp b/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
index 6e51f25a66407..746ba7bcea015 100644
--- a/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
+++ b/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
@@ -455,9 +455,9 @@ int includeFixerMain(int argc, const char **argv) {
}
// Set up a new source manager for applying the resulting replacements.
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
- DiagnosticsEngine Diagnostics(new DiagnosticIDs, &*DiagOpts);
- TextDiagnosticPrinter DiagnosticPrinter(outs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine Diagnostics(new DiagnosticIDs, DiagOpts);
+ TextDiagnosticPrinter DiagnosticPrinter(outs(), DiagOpts);
SourceManager SM(Diagnostics, tool.getFiles());
Diagnostics.setClient(&DiagnosticPrinter, false);
diff --git a/clang-tools-extra/clang-move/tool/ClangMove.cpp b/clang-tools-extra/clang-move/tool/ClangMove.cpp
index 655ea81ee37d4..750eb952714f7 100644
--- a/clang-tools-extra/clang-move/tool/ClangMove.cpp
+++ b/clang-tools-extra/clang-move/tool/ClangMove.cpp
@@ -176,10 +176,10 @@ int main(int argc, const char **argv) {
}
}
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
- clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
SourceManager SM(Diagnostics, FileMgr);
diff --git a/clang-tools-extra/clang-query/Query.cpp b/clang-tools-extra/clang-query/Query.cpp
index 382aa5d6fe25e..574b64ee0f759 100644
--- a/clang-tools-extra/clang-query/Query.cpp
+++ b/clang-tools-extra/clang-query/Query.cpp
@@ -172,7 +172,7 @@ bool MatchQuery::run(llvm::raw_ostream &OS, QuerySession &QS) const {
clang::SourceRange R = BI->second.getSourceRange();
if (R.isValid()) {
TextDiagnostic TD(OS, AST->getASTContext().getLangOpts(),
- &AST->getDiagnostics().getDiagnosticOptions());
+ AST->getDiagnostics().getDiagnosticOptions());
TD.emitDiagnostic(
FullSourceLoc(R.getBegin(), AST->getSourceManager()),
DiagnosticsEngine::Note, "\"" + BI->first + "\" binds here",
diff --git a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
index 5b77ee7b5738c..03502525417b2 100644
--- a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
+++ b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
@@ -72,10 +72,10 @@ int main(int argc, const char **argv) {
int ExitCode = Tool.run(Factory.get());
LangOptions DefaultLangOptions;
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
- TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 733a53a0f5dcc..26f9afbc0880e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -97,15 +97,14 @@ class ErrorReporter {
ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
: Files(FileSystemOptions(), std::move(BaseFS)),
- DiagOpts(new DiagnosticOptions()),
- DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
- Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
+ DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), DiagOpts)),
+ Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), DiagOpts,
DiagPrinter),
SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes) {
- DiagOpts->ShowColors = Context.getOptions().UseColor.value_or(
+ DiagOpts.ShowColors = Context.getOptions().UseColor.value_or(
llvm::sys::Process::StandardOutHasColors());
DiagPrinter->BeginSourceFile(LangOpts);
- if (DiagOpts->ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
+ if (DiagOpts.ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
llvm::sys::Process::UseANSIEscapeCodes(true);
}
}
@@ -308,7 +307,7 @@ class ErrorReporter {
FileManager Files;
LangOptions LangOpts; // FIXME: use langopts from each original file
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
+ DiagnosticOptions DiagOpts;
DiagnosticConsumer *DiagPrinter;
DiagnosticsEngine Diags;
SourceManager SourceMgr;
@@ -516,10 +515,10 @@ getCheckOptions(const ClangTidyOptions &Options,
Options),
AllowEnablingAnalyzerAlphaCheckers);
ClangTidyDiagnosticConsumer DiagConsumer(Context);
- DiagnosticsEngine DE(llvm::makeIntrusiveRefCnt<DiagnosticIDs>(),
- llvm::makeIntrusiveRefCnt<DiagnosticOptions>(),
+ auto DiagOpts = std::make_unique<DiagnosticOptions>();
+ DiagnosticsEngine DE(llvm::makeIntrusiveRefCnt<DiagnosticIDs>(), *DiagOpts,
&DiagConsumer, /*ShouldOwnClient=*/false);
- Context.setDiagnosticsEngine(&DE);
+ Context.setDiagnosticsEngine(std::move(DiagOpts), &DE);
ClangTidyASTConsumerFactory Factory(Context);
return Factory.getCheckOptions();
}
@@ -558,9 +557,10 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
Context.setProfileStoragePrefix(StoreCheckProfile);
ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, true, ApplyAnyFix);
- DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
- &DiagConsumer, /*ShouldOwnClient=*/false);
- Context.setDiagnosticsEngine(&DE);
+ auto DiagOpts = std::make_unique<DiagnosticOptions>();
+ DiagnosticsEngine DE(new DiagnosticIDs(), *DiagOpts, &DiagConsumer,
+ /*ShouldOwnClient=*/false);
+ Context.setDiagnosticsEngine(std::move(DiagOpts), &DE);
Tool.setDiagnosticConsumer(&DiagConsumer);
class ActionFactory : public FrontendActionFactory {
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..a0253a5fd1a48 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -49,7 +49,7 @@ namespace {
class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
public:
ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
- DiagnosticOptions *DiagOpts,
+ DiagnosticOptions &DiagOpts,
ClangTidyError &Error)
: DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
@@ -429,7 +429,7 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
forwardDiagnostic(Info);
} else {
ClangTidyDiagnosticRenderer Converter(
- Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
+ Context.getLangOpts(), Context.DiagEngine->getDiagnosticOptions(),
Errors.back());
SmallString<100> Message;
Info.FormatDiagnostic(Message);
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index d6cf6a2b2731e..bd7a1bf2c11c7 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -75,7 +75,9 @@ class ClangTidyContext {
/// Sets the DiagnosticsEngine that diag() will emit diagnostics to.
// FIXME: this is required initialization, and should be a constructor param.
// Fix the context -> diag engine -> consumer -> context initialization cycle.
- void setDiagnosticsEngine(DiagnosticsEngine *DiagEngine) {
+ void setDiagnosticsEngine(std::unique_ptr<DiagnosticOptions> DiagOpts,
+ DiagnosticsEngine *DiagEngine) {
+ this->DiagOpts = std::move(DiagOpts);
this->DiagEngine = DiagEngine;
}
@@ -231,6 +233,7 @@ class ClangTidyContext {
// Writes to Stats.
friend class ClangTidyDiagnosticConsumer;
+ std::unique_ptr<DiagnosticOptions> DiagOpts = nullptr;
DiagnosticsEngine *DiagEngine = nullptr;
std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index 03a3e8404e069..6a84704434c33 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -71,7 +71,7 @@ ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
InMemoryFs(new llvm::vfs::InMemoryFileSystem),
Sources(Compiler.getSourceManager()),
// Forward the new diagnostics to the original DiagnosticConsumer.
- Diags(new DiagnosticIDs, new DiagnosticOptions,
+ Diags(new DiagnosticIDs, DiagOpts,
new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
LangOpts(Compiler.getLangOpts()), HSOpts(Compiler.getHeaderSearchOpts()) {
// Add a FileSystem containing the extra files needed in place of modular
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
index a263681b3c633..c3478917ef498 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
@@ -128,6 +128,7 @@ class ExpandModularHeadersPPCallbacks : public PPCallbacks {
llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFs;
SourceManager &Sources;
+ DiagnosticOptions DiagOpts;
DiagnosticsEngine Diags;
LangOptions LangOpts;
HeaderSearchOptions HSOpts;
diff --git a/clang-tools-extra/clangd/Compiler.cpp b/clang-tools-extra/clangd/Compiler.cpp
index 9be0152afd2f7..8b3865c8a8e5c 100644
--- a/clang-tools-extra/clangd/Compiler.cpp
+++ b/clang-tools-extra/clangd/Compiler.cpp
@@ -110,8 +110,9 @@ buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
CIOpts.VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
CIOpts.CC1Args = CC1Args;
CIOpts.RecoverOnError = true;
- CIOpts.Diags = CompilerInstance::createDiagnostics(
- *CIOpts.VFS, new DiagnosticOptions, &D, false);
+ DiagnosticOptions DiagOpts;
+ CIOpts.Diags =
+ CompilerInstance::createDiagnostics(*CIOpts.VFS, DiagOpts, &D, false);
CIOpts.ProbePrecompiled = false;
std::unique_ptr<CompilerInvocation> CI = createInvocation(ArgStrs, CIOpts);
if (!CI)
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index c1878f91b5e16..bf77f43bd28bb 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -187,9 +187,9 @@ bool IsModuleFileUpToDate(PathRef ModuleFilePath,
HSOpts.ValidateASTInputFilesContent = true;
clang::clangd::IgnoreDiagnostics IgnoreDiags;
+ DiagnosticOptions DiagOpts;
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
- CompilerInstance::createDiagnostics(*VFS, new DiagnosticOptions,
- &IgnoreDiags,
+ CompilerInstance::createDiagnostics(*VFS, DiagOpts, &IgnoreDiags,
/*ShouldOwnClient=*/false);
LangOptions LangOpts;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3f63daaf400db..9e1f6bb977226 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -556,7 +556,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
*AllCTFactories, Cfg.Diagnostics.ClangTidy.FastCheckFilter);
CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
- CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
+ // The lifetime of DiagnosticOptions is managed by \c Clang.
+ CTContext->setDiagnosticsEngine(nullptr, &Clang->getDiagnostics());
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(Filename);
CTContext->setSelfContainedDiags(true);
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index ba9a53db8a0dc..7b4d63ff197e7 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -615,7 +615,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
});
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
- CompilerInstance::createDiagnostics(*VFS, &CI.getDiagnosticOpts(),
+ CompilerInstance::createDiagnostics(*VFS, CI.getDiagnosticOpts(),
&PreambleDiagnostics,
/*ShouldOwnClient=*/false);
const Config &Cfg = Config::current();
diff --git a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
index 6417bf8765622..0b067e8b0b2b2 100644
--- a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -253,7 +253,8 @@ namespace {
bool isValidTarget(llvm::StringRef Triple) {
std::shared_ptr<TargetOptions> TargetOpts(new TargetOptions);
TargetOpts->Triple = Triple.str();
- DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine Diags(new DiagnosticIDs, DiagOpts,
new IgnoringDiagConsumer);
llvm::IntrusiveRefCntPtr<TargetInfo> Target =
TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index c3e484a1a79c4..75d0ff244038d 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -298,7 +298,8 @@ TEST_F(ConfigCompileTests, DiagnosticSuppression) {
"unreachable-code", "unused-variable",
"typecheck_bool_condition",
"unexpected_friend", "warn_alloca"));
- clang::DiagnosticsEngine DiagEngine(new DiagnosticIDs, nullptr,
+ clang::DiagnosticOptions DiagOpts;
+ clang::DiagnosticsEngine DiagEngine(new DiagnosticIDs, DiagOpts,
new clang::IgnoringDiagConsumer);
using Diag = clang::Diagnostic;
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
index 8bd40c1429012..e39b70224d97c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
@@ -44,7 +44,8 @@ TEST(FileEdits, AbsolutePath) {
for (const auto *Path : RelPaths)
MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
FileManager FM(FileSystemOptions(), MemFS);
- DiagnosticsEngine DE(new DiagnosticIDs, new DiagnosticOptions);
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine DE(new DiagnosticIDs, DiagOpts);
SourceManager SM(DE, FM);
for (const auto *Path : RelPaths) {
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index a10c0d5a54a95..91d2697712b6e 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -618,8 +618,8 @@ TEST_F(PragmaIncludeTest, ExportInUnnamedBuffer) {
llvm::MemoryBuffer::getMemBufferCopy(Extra.getValue(),
/*BufferName=*/""));
- auto DiagOpts = llvm::makeIntrusiveRefCnt<DiagnosticOptions>();
- auto Diags = CompilerInstance::createDiagnostics(*VFS, DiagOpts.get());
+ DiagnosticOptions DiagOpts;
+ auto Diags = CompilerInstance::createDiagnostics(*VFS, DiagOpts);
auto Invocation = std::make_unique<CompilerInvocation>();
ASSERT_TRUE(CompilerInvocation::CreateFromArgs(*Invocation, {Filename.data()},
*Diags, "clang"));
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index e45ea36f7938e..5223eb563e4cb 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -85,9 +85,9 @@ std::vector<Decl::Kind> testWalk(llvm::StringRef TargetCode,
// For each difference, show the target point in context, like a diagnostic.
std::string DiagBuf;
llvm::raw_string_ostream DiagOS(DiagBuf);
- auto *DiagOpts = new DiagnosticOptions();
- DiagOpts->ShowLevel = 0;
- DiagOpts->ShowNoteIncludeStack = 0;
+ DiagnosticOptions DiagOpts;
+ DiagOpts.ShowLevel = 0;
+ DiagOpts.ShowNoteIncludeStack = 0;
TextDiagnostic Diag(DiagOS, AST.context().getLangOpts(), DiagOpts);
auto DiagnosePoint = [&](llvm::StringRef Message, unsigned Offset) {
Diag.emitDiagnostic(
diff --git a/clang-tools-extra/modularize/ModularizeUtilities.cpp b/clang-tools-extra/modularize/ModularizeUtilities.cpp
index 576e863c8a9d2..b04eb80a67717 100644
--- a/clang-tools-extra/modularize/ModularizeUtilities.cpp
+++ b/clang-tools-extra/modularize/ModularizeUtilities.cpp
@@ -48,10 +48,8 @@ ModularizeUtilities::ModularizeUtilities(...
[truncated]
|
|
@llvm/pr-subscribers-hlsl Author: Jan Svoboda (jansvoboda11) ChangesThe void shareOwnership(CompilerInvocation &CI) {
llvm::IntrusiveRefCntPtr<DiagnosticOptions> CoOwner = &CI.getDiagnosticOptions();
// ...
}This is a perfectly valid pattern that is being actually used in the codebase. I would like to ensure the ownership of Patch is 189.21 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139584.diff 134 Files Affected:
diff --git a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
index 68b5743c6540f..062e236d3e51f 100644
--- a/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ b/clang-tools-extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -96,9 +96,9 @@ int main(int argc, char **argv) {
cl::SetVersionPrinter(printVersion);
cl::ParseCommandLineOptions(argc, argv);
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
+ DiagnosticOptions DiagOpts;
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts.get());
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts);
// Determine a formatting style from options.
auto FormatStyleOrError = format::getStyle(FormatStyleOpt, FormatStyleConfig,
diff --git a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
index 22d26db0c11bc..2a8fe2d06d185 100644
--- a/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
+++ b/clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
@@ -126,10 +126,10 @@ int main(int argc, const char **argv) {
if (int Result = Tool.run(Factory.get()))
return Result;
LangOptions DefaultLangOptions;
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
- clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
SourceManager Sources(Diagnostics, FileMgr);
diff --git a/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp b/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
index 6e51f25a66407..746ba7bcea015 100644
--- a/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
+++ b/clang-tools-extra/clang-include-fixer/tool/ClangIncludeFixer.cpp
@@ -455,9 +455,9 @@ int includeFixerMain(int argc, const char **argv) {
}
// Set up a new source manager for applying the resulting replacements.
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions);
- DiagnosticsEngine Diagnostics(new DiagnosticIDs, &*DiagOpts);
- TextDiagnosticPrinter DiagnosticPrinter(outs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine Diagnostics(new DiagnosticIDs, DiagOpts);
+ TextDiagnosticPrinter DiagnosticPrinter(outs(), DiagOpts);
SourceManager SM(Diagnostics, tool.getFiles());
Diagnostics.setClient(&DiagnosticPrinter, false);
diff --git a/clang-tools-extra/clang-move/tool/ClangMove.cpp b/clang-tools-extra/clang-move/tool/ClangMove.cpp
index 655ea81ee37d4..750eb952714f7 100644
--- a/clang-tools-extra/clang-move/tool/ClangMove.cpp
+++ b/clang-tools-extra/clang-move/tool/ClangMove.cpp
@@ -176,10 +176,10 @@ int main(int argc, const char **argv) {
}
}
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
- clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ clang::TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
SourceManager SM(Diagnostics, FileMgr);
diff --git a/clang-tools-extra/clang-query/Query.cpp b/clang-tools-extra/clang-query/Query.cpp
index 382aa5d6fe25e..574b64ee0f759 100644
--- a/clang-tools-extra/clang-query/Query.cpp
+++ b/clang-tools-extra/clang-query/Query.cpp
@@ -172,7 +172,7 @@ bool MatchQuery::run(llvm::raw_ostream &OS, QuerySession &QS) const {
clang::SourceRange R = BI->second.getSourceRange();
if (R.isValid()) {
TextDiagnostic TD(OS, AST->getASTContext().getLangOpts(),
- &AST->getDiagnostics().getDiagnosticOptions());
+ AST->getDiagnostics().getDiagnosticOptions());
TD.emitDiagnostic(
FullSourceLoc(R.getBegin(), AST->getSourceManager()),
DiagnosticsEngine::Note, "\"" + BI->first + "\" binds here",
diff --git a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
index 5b77ee7b5738c..03502525417b2 100644
--- a/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
+++ b/clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
@@ -72,10 +72,10 @@ int main(int argc, const char **argv) {
int ExitCode = Tool.run(Factory.get());
LangOptions DefaultLangOptions;
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts(new DiagnosticOptions());
- TextDiagnosticPrinter DiagnosticPrinter(errs(), &*DiagOpts);
+ DiagnosticOptions DiagOpts;
+ TextDiagnosticPrinter DiagnosticPrinter(errs(), DiagOpts);
DiagnosticsEngine Diagnostics(
- IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
+ IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), DiagOpts,
&DiagnosticPrinter, false);
auto &FileMgr = Tool.getFiles();
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 733a53a0f5dcc..26f9afbc0880e 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -97,15 +97,14 @@ class ErrorReporter {
ErrorReporter(ClangTidyContext &Context, FixBehaviour ApplyFixes,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> BaseFS)
: Files(FileSystemOptions(), std::move(BaseFS)),
- DiagOpts(new DiagnosticOptions()),
- DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
- Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
+ DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), DiagOpts)),
+ Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), DiagOpts,
DiagPrinter),
SourceMgr(Diags, Files), Context(Context), ApplyFixes(ApplyFixes) {
- DiagOpts->ShowColors = Context.getOptions().UseColor.value_or(
+ DiagOpts.ShowColors = Context.getOptions().UseColor.value_or(
llvm::sys::Process::StandardOutHasColors());
DiagPrinter->BeginSourceFile(LangOpts);
- if (DiagOpts->ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
+ if (DiagOpts.ShowColors && !llvm::sys::Process::StandardOutIsDisplayed()) {
llvm::sys::Process::UseANSIEscapeCodes(true);
}
}
@@ -308,7 +307,7 @@ class ErrorReporter {
FileManager Files;
LangOptions LangOpts; // FIXME: use langopts from each original file
- IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
+ DiagnosticOptions DiagOpts;
DiagnosticConsumer *DiagPrinter;
DiagnosticsEngine Diags;
SourceManager SourceMgr;
@@ -516,10 +515,10 @@ getCheckOptions(const ClangTidyOptions &Options,
Options),
AllowEnablingAnalyzerAlphaCheckers);
ClangTidyDiagnosticConsumer DiagConsumer(Context);
- DiagnosticsEngine DE(llvm::makeIntrusiveRefCnt<DiagnosticIDs>(),
- llvm::makeIntrusiveRefCnt<DiagnosticOptions>(),
+ auto DiagOpts = std::make_unique<DiagnosticOptions>();
+ DiagnosticsEngine DE(llvm::makeIntrusiveRefCnt<DiagnosticIDs>(), *DiagOpts,
&DiagConsumer, /*ShouldOwnClient=*/false);
- Context.setDiagnosticsEngine(&DE);
+ Context.setDiagnosticsEngine(std::move(DiagOpts), &DE);
ClangTidyASTConsumerFactory Factory(Context);
return Factory.getCheckOptions();
}
@@ -558,9 +557,10 @@ runClangTidy(clang::tidy::ClangTidyContext &Context,
Context.setProfileStoragePrefix(StoreCheckProfile);
ClangTidyDiagnosticConsumer DiagConsumer(Context, nullptr, true, ApplyAnyFix);
- DiagnosticsEngine DE(new DiagnosticIDs(), new DiagnosticOptions(),
- &DiagConsumer, /*ShouldOwnClient=*/false);
- Context.setDiagnosticsEngine(&DE);
+ auto DiagOpts = std::make_unique<DiagnosticOptions>();
+ DiagnosticsEngine DE(new DiagnosticIDs(), *DiagOpts, &DiagConsumer,
+ /*ShouldOwnClient=*/false);
+ Context.setDiagnosticsEngine(std::move(DiagOpts), &DE);
Tool.setDiagnosticConsumer(&DiagConsumer);
class ActionFactory : public FrontendActionFactory {
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index b216970bfbd8c..a0253a5fd1a48 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -49,7 +49,7 @@ namespace {
class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
public:
ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
- DiagnosticOptions *DiagOpts,
+ DiagnosticOptions &DiagOpts,
ClangTidyError &Error)
: DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
@@ -429,7 +429,7 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic(
forwardDiagnostic(Info);
} else {
ClangTidyDiagnosticRenderer Converter(
- Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
+ Context.getLangOpts(), Context.DiagEngine->getDiagnosticOptions(),
Errors.back());
SmallString<100> Message;
Info.FormatDiagnostic(Message);
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index d6cf6a2b2731e..bd7a1bf2c11c7 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -75,7 +75,9 @@ class ClangTidyContext {
/// Sets the DiagnosticsEngine that diag() will emit diagnostics to.
// FIXME: this is required initialization, and should be a constructor param.
// Fix the context -> diag engine -> consumer -> context initialization cycle.
- void setDiagnosticsEngine(DiagnosticsEngine *DiagEngine) {
+ void setDiagnosticsEngine(std::unique_ptr<DiagnosticOptions> DiagOpts,
+ DiagnosticsEngine *DiagEngine) {
+ this->DiagOpts = std::move(DiagOpts);
this->DiagEngine = DiagEngine;
}
@@ -231,6 +233,7 @@ class ClangTidyContext {
// Writes to Stats.
friend class ClangTidyDiagnosticConsumer;
+ std::unique_ptr<DiagnosticOptions> DiagOpts = nullptr;
DiagnosticsEngine *DiagEngine = nullptr;
std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
index 03a3e8404e069..6a84704434c33 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -71,7 +71,7 @@ ExpandModularHeadersPPCallbacks::ExpandModularHeadersPPCallbacks(
InMemoryFs(new llvm::vfs::InMemoryFileSystem),
Sources(Compiler.getSourceManager()),
// Forward the new diagnostics to the original DiagnosticConsumer.
- Diags(new DiagnosticIDs, new DiagnosticOptions,
+ Diags(new DiagnosticIDs, DiagOpts,
new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
LangOpts(Compiler.getLangOpts()), HSOpts(Compiler.getHeaderSearchOpts()) {
// Add a FileSystem containing the extra files needed in place of modular
diff --git a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
index a263681b3c633..c3478917ef498 100644
--- a/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
+++ b/clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.h
@@ -128,6 +128,7 @@ class ExpandModularHeadersPPCallbacks : public PPCallbacks {
llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFs;
SourceManager &Sources;
+ DiagnosticOptions DiagOpts;
DiagnosticsEngine Diags;
LangOptions LangOpts;
HeaderSearchOptions HSOpts;
diff --git a/clang-tools-extra/clangd/Compiler.cpp b/clang-tools-extra/clangd/Compiler.cpp
index 9be0152afd2f7..8b3865c8a8e5c 100644
--- a/clang-tools-extra/clangd/Compiler.cpp
+++ b/clang-tools-extra/clangd/Compiler.cpp
@@ -110,8 +110,9 @@ buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
CIOpts.VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
CIOpts.CC1Args = CC1Args;
CIOpts.RecoverOnError = true;
- CIOpts.Diags = CompilerInstance::createDiagnostics(
- *CIOpts.VFS, new DiagnosticOptions, &D, false);
+ DiagnosticOptions DiagOpts;
+ CIOpts.Diags =
+ CompilerInstance::createDiagnostics(*CIOpts.VFS, DiagOpts, &D, false);
CIOpts.ProbePrecompiled = false;
std::unique_ptr<CompilerInvocation> CI = createInvocation(ArgStrs, CIOpts);
if (!CI)
diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index c1878f91b5e16..bf77f43bd28bb 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -187,9 +187,9 @@ bool IsModuleFileUpToDate(PathRef ModuleFilePath,
HSOpts.ValidateASTInputFilesContent = true;
clang::clangd::IgnoreDiagnostics IgnoreDiags;
+ DiagnosticOptions DiagOpts;
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
- CompilerInstance::createDiagnostics(*VFS, new DiagnosticOptions,
- &IgnoreDiags,
+ CompilerInstance::createDiagnostics(*VFS, DiagOpts, &IgnoreDiags,
/*ShouldOwnClient=*/false);
LangOptions LangOpts;
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 3f63daaf400db..9e1f6bb977226 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -556,7 +556,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
*AllCTFactories, Cfg.Diagnostics.ClangTidy.FastCheckFilter);
CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
- CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
+ // The lifetime of DiagnosticOptions is managed by \c Clang.
+ CTContext->setDiagnosticsEngine(nullptr, &Clang->getDiagnostics());
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(Filename);
CTContext->setSelfContainedDiags(true);
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index ba9a53db8a0dc..7b4d63ff197e7 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -615,7 +615,7 @@ buildPreamble(PathRef FileName, CompilerInvocation CI,
});
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
- CompilerInstance::createDiagnostics(*VFS, &CI.getDiagnosticOpts(),
+ CompilerInstance::createDiagnostics(*VFS, CI.getDiagnosticOpts(),
&PreambleDiagnostics,
/*ShouldOwnClient=*/false);
const Config &Cfg = Config::current();
diff --git a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
index 6417bf8765622..0b067e8b0b2b2 100644
--- a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -253,7 +253,8 @@ namespace {
bool isValidTarget(llvm::StringRef Triple) {
std::shared_ptr<TargetOptions> TargetOpts(new TargetOptions);
TargetOpts->Triple = Triple.str();
- DiagnosticsEngine Diags(new DiagnosticIDs, new DiagnosticOptions,
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine Diags(new DiagnosticIDs, DiagOpts,
new IgnoringDiagConsumer);
llvm::IntrusiveRefCntPtr<TargetInfo> Target =
TargetInfo::CreateTargetInfo(Diags, *TargetOpts);
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index c3e484a1a79c4..75d0ff244038d 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -298,7 +298,8 @@ TEST_F(ConfigCompileTests, DiagnosticSuppression) {
"unreachable-code", "unused-variable",
"typecheck_bool_condition",
"unexpected_friend", "warn_alloca"));
- clang::DiagnosticsEngine DiagEngine(new DiagnosticIDs, nullptr,
+ clang::DiagnosticOptions DiagOpts;
+ clang::DiagnosticsEngine DiagEngine(new DiagnosticIDs, DiagOpts,
new clang::IgnoringDiagConsumer);
using Diag = clang::Diagnostic;
diff --git a/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
index 8bd40c1429012..e39b70224d97c 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/TweakTests.cpp
@@ -44,7 +44,8 @@ TEST(FileEdits, AbsolutePath) {
for (const auto *Path : RelPaths)
MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
FileManager FM(FileSystemOptions(), MemFS);
- DiagnosticsEngine DE(new DiagnosticIDs, new DiagnosticOptions);
+ DiagnosticOptions DiagOpts;
+ DiagnosticsEngine DE(new DiagnosticIDs, DiagOpts);
SourceManager SM(DE, FM);
for (const auto *Path : RelPaths) {
diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
index a10c0d5a54a95..91d2697712b6e 100644
--- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
@@ -618,8 +618,8 @@ TEST_F(PragmaIncludeTest, ExportInUnnamedBuffer) {
llvm::MemoryBuffer::getMemBufferCopy(Extra.getValue(),
/*BufferName=*/""));
- auto DiagOpts = llvm::makeIntrusiveRefCnt<DiagnosticOptions>();
- auto Diags = CompilerInstance::createDiagnostics(*VFS, DiagOpts.get());
+ DiagnosticOptions DiagOpts;
+ auto Diags = CompilerInstance::createDiagnostics(*VFS, DiagOpts);
auto Invocation = std::make_unique<CompilerInvocation>();
ASSERT_TRUE(CompilerInvocation::CreateFromArgs(*Invocation, {Filename.data()},
*Diags, "clang"));
diff --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index e45ea36f7938e..5223eb563e4cb 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -85,9 +85,9 @@ std::vector<Decl::Kind> testWalk(llvm::StringRef TargetCode,
// For each difference, show the target point in context, like a diagnostic.
std::string DiagBuf;
llvm::raw_string_ostream DiagOS(DiagBuf);
- auto *DiagOpts = new DiagnosticOptions();
- DiagOpts->ShowLevel = 0;
- DiagOpts->ShowNoteIncludeStack = 0;
+ DiagnosticOptions DiagOpts;
+ DiagOpts.ShowLevel = 0;
+ DiagOpts.ShowNoteIncludeStack = 0;
TextDiagnostic Diag(DiagOS, AST.context().getLangOpts(), DiagOpts);
auto DiagnosePoint = [&](llvm::StringRef Message, unsigned Offset) {
Diag.emitDiagnostic(
diff --git a/clang-tools-extra/modularize/ModularizeUtilities.cpp b/clang-tools-extra/modularize/ModularizeUtilities.cpp
index 576e863c8a9d2..b04eb80a67717 100644
--- a/clang-tools-extra/modularize/ModularizeUtilities.cpp
+++ b/clang-tools-extra/modularize/ModularizeUtilities.cpp
@@ -48,10 +48,8 @@ ModularizeUtilities::ModularizeUtilities(...
[truncated]
|
cor3ntin
approved these changes
May 14, 2025
ojhunt
reviewed
May 14, 2025
| @@ -2032,6 +2032,7 @@ class SourceManagerForFile { | |||
| // as they are created in `createSourceManagerForFile` so that they can be | |||
| // deleted in the reverse order as they are created. | |||
| std::unique_ptr<FileManager> FileMgr; | |||
| std::unique_ptr<DiagnosticOptions> DiagOpts; | |||
There was a problem hiding this comment.
I don't like that in some cases we have a unique_ptr and later (ATSUnit) we use a shared_ptr<>. It leaves an author unable to know given some arbitrary DiagnosticOptions* what the lifetime and/or ownership rules are.
There was a problem hiding this comment.
I don't think I understand. Now that DiagnosticOptions are not intrusively reference-counted, raw pointers have clearer semantics than before (typically nullable non-owning borrow), no? I'd also argue that using values, references, raw pointers, unique_ptr and shared_ptr depending on the context is the clearest way to communicate lifetimes and ownership. Regardless, there's only one raw pointer to DiagnosticOptions remaining after my patch in clang::tooling::ToolInvocation, and that has exactly the semantics you'd expect: optional borrow where the owner is someone else and you expect them to keep the object alive long enough.
There was a problem hiding this comment.
@jansvoboda11 the problem is that now an author does not know if a given DiagnosticOption is a unique_ptr or a shared_ptr. A given DiagnosticOption* may or may not be protectable, etc depending on the context and origin of the object.
Philosophically I prefer intrusive refcounts over shared_ptr because to me they make the lifetime much clearer as the lifetime rules are embedded in the type, but I don't think that's an issue in this PR.
My understanding is that the goal of this PR is to say "For API purposes, a given DiagnosticOption reference is only live as long as the API object that vends it. As an implementation detail there are some cases where it can outlast the vending object, but that's not generally part of the user visible API."
That's a perfectly reasonable change, but my concern is that by mixing and matching shared and unique_ptr an author loses the ability to reason about what a given object's lifetime is. It seems like the reason for shared_ptr is to deal with some slightly gross bits of the API, and I wonder if it's possible to fix those APIs so we can just use unique_ptr everywhere?
cor3ntin
reviewed
May 14, 2025
Bigcheese
approved these changes
May 22, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/19518 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/22979 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/25826 Here is the relevant piece of the build log for the reference |
|
@jansvoboda11 I've revered your PR due buildbot failures above (and my local build failures with the same error messages). I'm happy to try your revised patch to see if it build cleanly. Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/24607 Here is the relevant piece of the build log for the reference |
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/16354 Here is the relevant piece of the build log for the reference |
|
Reverted the revert, since the build now fails due to the forward-fixes not being reverted... |
Ah, I just realized. Thanks! Even if I reapply your patch, I still get: among other things. I'm not sure these are related to your PR. |
|
Are those in the upstream repo? If so, where are they coming from? I have a sparse checkout, so it's possible I missed some non-Clang and non-LLDB usages of |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/17038 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/20223 Here is the relevant piece of the build log for the reference |
|
@jansvoboda11 I also see: Another one: from from |
|
Thanks, I'll fix those ASAP. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 22, 2025
…gnosticOptions` (#139584)" This reverts commit 9e306ad. Multiple builtbot failures have been reported: llvm/llvm-project#139584
@jansvoboda11 Please feel free to add me as a reviewer. I am happy to build things with your fix. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/10167 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/10145 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/11354 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/32614 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/28805 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/28206 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/26567 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
DiagnosticOptionsclass is currently intrusively reference-counted, which makes reasoning about its lifetime very difficult in some cases. For example,CompilerInvocationowns theDiagnosticOptionsinstance (wrapped inllvm::IntrusiveRefCntPtr) and only exposes an accessor returningDiagnosticOptions &. One would think this givesCompilerInvocationexclusive ownership of the object, but that's not the case:This is a perfectly valid pattern that is being actually used in the codebase.
I would like to ensure the ownership of
DiagnosticOptionsbyCompilerInvocationis guaranteed to be exclusive. This can be leveraged for a copy-on-write optimization later on. This PR changes usages ofDiagnosticOptionsacrossclang,clang-tools-extraandlldbto not be intrusively reference-counted.