[clang] Hide the LangOptions pointer from CompilerInvocation
#137675
Merged
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
Member
|
@llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThis PR makes Full diff: https://github.com/llvm/llvm-project/pull/137675.diff 5 Files Affected:
diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index 2baa2d1cc540d..73686b0eacbe2 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -106,7 +106,7 @@ class ASTUnit {
};
private:
- std::shared_ptr<LangOptions> LangOpts;
+ std::unique_ptr<LangOptions> LangOpts;
IntrusiveRefCntPtr<DiagnosticsEngine> Diagnostics;
IntrusiveRefCntPtr<FileManager> FileMgr;
IntrusiveRefCntPtr<SourceManager> SourceMgr;
@@ -704,7 +704,7 @@ class ASTUnit {
IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
const FileSystemOptions &FileSystemOpts,
const HeaderSearchOptions &HSOpts,
- std::shared_ptr<LangOptions> LangOpts = nullptr,
+ const LangOptions *LangOpts = nullptr,
bool OnlyLocalDecls = false,
CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
bool AllowASTWithCompilerErrors = false,
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 7de4fb531d0e7..78ac1079354de 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -327,9 +327,6 @@ class CompilerInstance : public ModuleLoader {
LangOptions &getLangOpts() { return Invocation->getLangOpts(); }
const LangOptions &getLangOpts() const { return Invocation->getLangOpts(); }
- std::shared_ptr<LangOptions> getLangOptsPtr() const {
- return Invocation->getLangOptsPtr();
- }
PreprocessorOptions &getPreprocessorOpts() {
return Invocation->getPreprocessorOpts();
diff --git a/clang/include/clang/Frontend/CompilerInvocation.h b/clang/include/clang/Frontend/CompilerInvocation.h
index 1827ff0f6816d..31bf7e94efab8 100644
--- a/clang/include/clang/Frontend/CompilerInvocation.h
+++ b/clang/include/clang/Frontend/CompilerInvocation.h
@@ -265,12 +265,6 @@ class CompilerInvocation : public CompilerInvocationBase {
}
/// @}
- /// Base class internals.
- /// @{
- using CompilerInvocationBase::LangOpts;
- std::shared_ptr<LangOptions> getLangOptsPtr() { return LangOpts; }
- /// @}
-
/// Create a compiler invocation from a list of input options.
/// \returns true on success.
///
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 3e4da76916585..e05385d119870 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -805,7 +805,7 @@ std::unique_ptr<ASTUnit> ASTUnit::LoadFromASTFile(
StringRef Filename, const PCHContainerReader &PCHContainerRdr,
WhatToLoad ToLoad, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
const FileSystemOptions &FileSystemOpts, const HeaderSearchOptions &HSOpts,
- std::shared_ptr<LangOptions> LangOpts, bool OnlyLocalDecls,
+ const LangOptions *LangOpts, bool OnlyLocalDecls,
CaptureDiagsKind CaptureDiagnostics, bool AllowASTWithCompilerErrors,
bool UserFilesAreVolatile, IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
std::unique_ptr<ASTUnit> AST(new ASTUnit(true));
@@ -819,7 +819,8 @@ std::unique_ptr<ASTUnit> ASTUnit::LoadFromASTFile(
ConfigureDiags(Diags, *AST, CaptureDiagnostics);
- AST->LangOpts = LangOpts ? LangOpts : std::make_shared<LangOptions>();
+ AST->LangOpts = LangOpts ? std::make_unique<LangOptions>(*LangOpts)
+ : std::make_unique<LangOptions>();
AST->OnlyLocalDecls = OnlyLocalDecls;
AST->CaptureDiagnostics = CaptureDiagnostics;
AST->Diagnostics = Diags;
@@ -1211,7 +1212,8 @@ bool ASTUnit::Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
"IR inputs not support here!");
// Configure the various subsystems.
- LangOpts = Clang->getInvocation().LangOpts;
+ LangOpts =
+ std::make_unique<LangOptions>(Clang->getInvocation().getLangOpts());
FileSystemOpts = Clang->getFileSystemOpts();
ResetForParse();
@@ -1486,7 +1488,7 @@ void ASTUnit::transferASTDataFromCompilerInstance(CompilerInstance &CI) {
// Steal the created target, context, and preprocessor if they have been
// created.
assert(CI.hasInvocation() && "missing invocation");
- LangOpts = CI.getInvocation().LangOpts;
+ LangOpts = std::make_unique<LangOptions>(CI.getInvocation().getLangOpts());
TheSema = CI.takeSema();
Consumer = CI.takeASTConsumer();
if (CI.hasASTContext())
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 783d1a64132b6..9b2aa253c90ee 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -847,7 +847,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromASTFile(
InputFile, CI.getPCHContainerReader(), ASTUnit::LoadEverything, Diags,
- CI.getFileSystemOpts(), CI.getHeaderSearchOpts(), CI.getLangOptsPtr());
+ CI.getFileSystemOpts(), CI.getHeaderSearchOpts(), &CI.getLangOpts());
if (!AST)
return false;
|
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- clang/include/clang/Frontend/ASTUnit.h clang/include/clang/Frontend/CompilerInstance.h clang/include/clang/Frontend/CompilerInvocation.h clang/lib/Frontend/ASTUnit.cpp clang/lib/Frontend/FrontendAction.cppView the diff from clang-format here.diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index 73686b0ea..ac99f0fb2 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -698,19 +698,17 @@ public:
/// lifetime is expected to extend past that of the returned ASTUnit.
///
/// \returns - The initialized ASTUnit or null if the AST failed to load.
- static std::unique_ptr<ASTUnit>
- LoadFromASTFile(StringRef Filename, const PCHContainerReader &PCHContainerRdr,
- WhatToLoad ToLoad,
- IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
- const FileSystemOptions &FileSystemOpts,
- const HeaderSearchOptions &HSOpts,
- const LangOptions *LangOpts = nullptr,
- bool OnlyLocalDecls = false,
- CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
- bool AllowASTWithCompilerErrors = false,
- bool UserFilesAreVolatile = false,
- IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS =
- llvm::vfs::getRealFileSystem());
+ static std::unique_ptr<ASTUnit> LoadFromASTFile(
+ StringRef Filename, const PCHContainerReader &PCHContainerRdr,
+ WhatToLoad ToLoad, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
+ const FileSystemOptions &FileSystemOpts,
+ const HeaderSearchOptions &HSOpts, const LangOptions *LangOpts = nullptr,
+ bool OnlyLocalDecls = false,
+ CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
+ bool AllowASTWithCompilerErrors = false,
+ bool UserFilesAreVolatile = false,
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS =
+ llvm::vfs::getRealFileSystem());
private:
/// Helper function for \c LoadFromCompilerInvocation() and
|
Collaborator
|
I'm feeling stupid but how does it work with ASan? I'm confused because in
Seems like the ownership is shared between |
Contributor
Author
|
The |
Collaborator
And we need |
vsapsai
reviewed
Apr 29, 2025
Collaborator
vsapsai
left a comment
vsapsai
left a comment
There was a problem hiding this comment.
Also looks like this assert doesn't serve its value anymore
const LangOptions &getLangOpts() const {
assert(LangOpts && "ASTUnit does not have language options");
return *LangOpts;
}But it is a minor cleanup, not blocking.
Contributor
Author
Not really, using
|
Contributor
Author
I think it still does have a value because |
Collaborator
I think that is a reasonable choice. |
vsapsai
approved these changes
Apr 29, 2025
Collaborator
vsapsai
left a comment
vsapsai
left a comment
There was a problem hiding this comment.
Ok to merge after fixing the formatting.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/4385 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…m#137675) This PR makes `CompilerInvocation` the sole owner of the `LangOptions` instance.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…m#137675) This PR makes `CompilerInvocation` the sole owner of the `LangOptions` instance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes
CompilerInvocationthe sole owner of theLangOptionsinstance.