[clang] Remove intrusive reference count from DiagnosticOptions
#139584
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
|
|
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 |
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
May 24, 2025
comgr and driver.cpp changes due to 13e1a2c Reapply "[clang] Remove intrusive reference count from `DiagnosticOptions` (llvm#139584)"
|
I'm trying to update some non-in-tree code after this change, and I don't think I understand the newly-expected lifetime rules after this change. I see all the comments that say this makes lifetime clearer...so I expect I'm just misunderstanding things. Previously, DiagnosticsEngine kept a DiagnosticOptions member via IntrusiveRefCntPtr, which meant the DiagnosticsOptions lifetime would be at least as long as the DiagnosticsEngine. Now, it stores a reference. Which means, I think, that the creator of a DiagnosticsEngine needs to somehow ensure by themselves that the DiagnosticsOptions value has a sufficiently long lifetime. But this seems very hard to figure out what lifetimes should be now, because DiagnosticsEngine is, itself, refcounted. How does it make sense to have a borrowed reference to DiagnosticsOption in the refcounted DiagnosticsEngine type? ISTM the caller who creates a DiagnosticsEngine cannot now easily know what the lifetime of the DiagnosticsEngine is going to be, and thus, has no way to ensure the DiagnosticsOptions lifetime is correct? |
|
@jyknight If the lifetime of the |
|
The problem is, it's hard to tell if it's clearly bounded or not. The code is passing a DiagnosticsEngine to various Clang APIs in CompilerInvocation/CompilerInstance areas, and how do I know whether those are expecting to keep the DiagnosticsEngine or not -- since it is, after all, an intrusive refcounted object! Only after a bunch of code delving and exploration, is it possible to figure out. Let's take this one example, and some of my thought process around it: I look at this and want to figure out what change I need to make. I see in some other code in this PR, the Well, let me try to figure it out. The code first passes the DiagnosticsEngine to createInvocation. Next thing is using the Invocation to create a CompilerInstance. We call Fine. So, where should I keep my DiagnosticOptions now? It looks like CompilerInvocation actually has a And here is where I am now. Though now I'm thinking in this case maybe I can just drop the DiagnosticsEngine code in this snippet entirely since the default seems already to be to print to the But should this really be this hard to figure out? |
|
Thanks for the concrete example! I think the key thing to realize is that the So concretely, I'd rewrite your example as: clang::DiagnosticOptions diagnostic_options;
llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diagnostics_engine =
clang::CompilerInstance::createDiagnostics(
*file_system, diagnostic_options,
new clang::TextDiagnosticPrinter(llvm::errs(), diagnostic_options));
clang::CreateInvocationOptions ci_opts;
ci_opts.Diags = diagnostics_engine;
// ...
std::shared_ptr<clang::CompilerInvocation> invocation =
clang::createInvocation(args, ci_opts);
auto compiler_instance =
std::make_unique<clang::CompilerInstance>(invocation);
compiler_instance->createDiagnostics(*file_system));
// ...
return compiler_instance;I agree the lifetimes shouldn't be this complicated. FWIW I'd like to get rid of the reference count of |
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…vm#139584) The `DiagnosticOptions` class is currently intrusively reference-counted, which makes reasoning about its lifetime very difficult in some cases. For example, `CompilerInvocation` owns the `DiagnosticOptions` instance (wrapped in `llvm::IntrusiveRefCntPtr`) and only exposes an accessor returning `DiagnosticOptions &`. One would think this gives `CompilerInvocation` exclusive ownership of the object, but that's not the case: ```c++ 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 `DiagnosticOptions` by `CompilerInvocation` is guaranteed to be exclusive. This can be leveraged for a copy-on-write optimization later on. This PR changes usages of `DiagnosticOptions` across `clang`, `clang-tools-extra` and `lldb` to not be intrusively reference-counted.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…ons` (llvm#139584)" This reverts commit 9e306ad. Multiple builtbot failures have been reported: llvm#139584
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…ions` (llvm#139584)" This reverts commit e2a8855. Build failures were fixed right away and reverting the original commit without the fixes breaks the build again.
github-merge-queue bot
pushed a commit
to carbon-language/carbon-lang
that referenced
this pull request
Jun 4, 2025
Some updates required for llvm/llvm-project#139584. Co-authored-by: Josh L <[email protected]>
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.