[clang] Be const-correct with all uses of Module *. #93493
Conversation
llvmbot added clang 
| @llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang Author: David Stone (davidstone) ChangesPatch is 129.44 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93493.diff 59 Files Affected:
diff --git a/clang/include/clang/APINotes/APINotesManager.h b/clang/include/clang/APINotes/APINotesManager.h index 18375c9e51a17..b559c24b322f2 100644 --- a/clang/include/clang/APINotes/APINotesManager.h +++ b/clang/include/clang/APINotes/APINotesManager.h @@ -145,7 +145,7 @@ class APINotesManager { /// /// \returns a vector of FileEntry where APINotes files are. llvm::SmallVector<FileEntryRef, 2> - getCurrentModuleAPINotes(Module *M, bool LookInModule, + getCurrentModuleAPINotes(const Module *M, bool LookInModule, ArrayRef<std::string> SearchPaths); /// Load Compiled API notes for current module. diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index a1d1d1c51cd41..70d131c12e073 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -462,7 +462,7 @@ class ASTContext : public RefCountedBase<ASTContext> { void resolve(ASTContext &Ctx); }; - llvm::DenseMap<Module*, PerModuleInitializers*> ModuleInitializers; + llvm::DenseMap<const Module *, PerModuleInitializers *> ModuleInitializers; /// This is the top-level (C++20) Named module we are building. Module *CurrentCXXNamedModule = nullptr; @@ -1060,12 +1060,12 @@ class ASTContext : public RefCountedBase<ASTContext> { /// for a module. This will typically be a global variable (with internal /// linkage) that runs module initializers, such as the iostream initializer, /// or an ImportDecl nominating another module that has initializers. - void addModuleInitializer(Module *M, Decl *Init); + void addModuleInitializer(const Module *M, Decl *Init); - void addLazyModuleInitializers(Module *M, ArrayRef<GlobalDeclID> IDs); + void addLazyModuleInitializers(const Module *M, ArrayRef<GlobalDeclID> IDs); /// Get the initializations to perform when importing a module, if any. - ArrayRef<Decl*> getModuleInitializers(Module *M); + ArrayRef<Decl *> getModuleInitializers(const Module *M); /// Set the (C++20) module we are building. void setCurrentNamedModule(Module *M); diff --git a/clang/include/clang/AST/ASTMutationListener.h b/clang/include/clang/AST/ASTMutationListener.h index 2c4ec2ce67f36..fc476f531815c 100644 --- a/clang/include/clang/AST/ASTMutationListener.h +++ b/clang/include/clang/AST/ASTMutationListener.h @@ -139,7 +139,7 @@ class ASTMutationListener { /// \param D The definition that was previously not visible. /// \param M The containing module in which the definition was made visible, /// if any. - virtual void RedefinedHiddenDefinition(const NamedDecl *D, Module *M) {} + virtual void RedefinedHiddenDefinition(const NamedDecl *D, const Module *M) {} /// An attribute was added to a RecordDecl /// diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index e43e812cd9455..e7dac0c67ff3d 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -817,11 +817,11 @@ class alignas(8) Decl { "owned local decl but no local module storage"); return reinterpret_cast<Module *const *>(this)[-1]; } - void setLocalOwningModule(Module *M) { + void setLocalOwningModule(const Module *M) { assert(!isFromASTFile() && hasOwningModule() && hasLocalOwningModuleStorage() && "should not have a cached owning module"); - reinterpret_cast<Module **>(this)[-1] = M; + reinterpret_cast<const Module **>(this)[-1] = M; } /// Is this declaration owned by some module? @@ -839,7 +839,7 @@ class alignas(8) Decl { /// /// \param IgnoreLinkage Ignore the linkage of the entity; assume that /// all declarations in a global module fragment are unowned. - Module *getOwningModuleForLinkage(bool IgnoreLinkage = false) const; + const Module *getOwningModuleForLinkage(bool IgnoreLinkage = false) const; /// Determine whether this declaration is definitely visible to name lookup, /// independent of whether the owning module is visible. diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h index 2d62d05cd9190..d85e59bd5e9bb 100644 --- a/clang/include/clang/Basic/Module.h +++ b/clang/include/clang/Basic/Module.h @@ -296,7 +296,7 @@ class alignas(8) Module { SmallVector<Requirement, 2> Requirements; /// A module with the same name that shadows this module. - Module *ShadowingModule = nullptr; + const Module *ShadowingModule = nullptr; /// Whether this module has declared itself unimportable, either because /// it's missing a requirement from \p Requirements or because it's been @@ -403,7 +403,7 @@ class alignas(8) Module { /// The set of top-level modules that affected the compilation of this module, /// but were not imported. - llvm::SmallSetVector<Module *, 2> AffectingClangModules; + llvm::SmallSetVector<const Module *, 2> AffectingClangModules; /// Describes an exported module. /// @@ -433,7 +433,7 @@ class alignas(8) Module { SmallVector<UnresolvedExportDecl, 2> UnresolvedExports; /// The directly used modules. - SmallVector<Module *, 2> DirectUses; + SmallVector<const Module *, 2> DirectUses; /// The set of use declarations that have yet to be resolved. SmallVector<ModuleId, 2> UnresolvedDirectUses; @@ -487,7 +487,7 @@ class alignas(8) Module { /// A conflict between two modules. struct Conflict { /// The module that this module conflicts with. - Module *Other; + const Module *Other; /// The message provided to the user when there is a conflict. std::string Message; @@ -519,7 +519,7 @@ class alignas(8) Module { /// \param ShadowingModule If this module is unimportable because it is /// shadowed, this parameter will be set to the shadowing module. bool isUnimportable(const LangOptions &LangOpts, const TargetInfo &Target, - Requirement &Req, Module *&ShadowingModule) const; + Requirement &Req, const Module *&ShadowingModule) const; /// Determine whether this module can be built in this compilation. bool isForBuilding(const LangOptions &LangOpts) const; @@ -545,11 +545,9 @@ class alignas(8) Module { /// /// \param ShadowingModule If this module is unavailable because it is /// shadowed, this parameter will be set to the shadowing module. - bool isAvailable(const LangOptions &LangOpts, - const TargetInfo &Target, - Requirement &Req, - UnresolvedHeaderDirective &MissingHeader, - Module *&ShadowingModule) const; + bool isAvailable(const LangOptions &LangOpts, const TargetInfo &Target, + Requirement &Req, UnresolvedHeaderDirective &MissingHeader, + const Module *&ShadowingModule) const; /// Determine whether this module is a submodule. bool isSubModule() const { return Parent != nullptr; } @@ -755,13 +753,13 @@ class alignas(8) Module { /// one. /// /// \returns The GMF sub-module if found, or NULL otherwise. - Module *getGlobalModuleFragment() const; + const Module *getGlobalModuleFragment() const; /// Get the Private Module Fragment (sub-module) for this module, it there is /// one. /// /// \returns The PMF sub-module if found, or NULL otherwise. - Module *getPrivateModuleFragment() const; + const Module *getPrivateModuleFragment() const; /// Determine whether the specified module would be visible to /// a lookup at the end of this module. @@ -845,20 +843,22 @@ class VisibleModuleSet { /// A callback to call when a module is made visible (directly or /// indirectly) by a call to \ref setVisible. - using VisibleCallback = llvm::function_ref<void(Module *M)>; + using VisibleCallback = llvm::function_ref<void(const Module *M)>; /// A callback to call when a module conflict is found. \p Path /// consists of a sequence of modules from the conflicting module to the one /// made visible, where each was exported by the next. using ConflictCallback = - llvm::function_ref<void(ArrayRef<Module *> Path, Module *Conflict, - StringRef Message)>; + llvm::function_ref<void(ArrayRef<const Module *> Path, + const Module *Conflict, StringRef Message)>; /// Make a specific module visible. - void setVisible(Module *M, SourceLocation Loc, - VisibleCallback Vis = [](Module *) {}, - ConflictCallback Cb = [](ArrayRef<Module *>, Module *, - StringRef) {}); + void setVisible( + const Module *M, SourceLocation Loc, + VisibleCallback Vis = [](const Module *) {}, + ConflictCallback Cb = [](ArrayRef<const Module *>, const Module *, + StringRef) {}); + private: /// Import locations for each visible module. Indexed by the module's /// VisibilityID. @@ -876,7 +876,7 @@ class ASTSourceDescriptor { StringRef Path; StringRef ASTFile; ASTFileSignature Signature; - Module *ClangModule = nullptr; + const Module *ClangModule = nullptr; public: ASTSourceDescriptor() = default; @@ -884,13 +884,13 @@ class ASTSourceDescriptor { ASTFileSignature Signature) : PCHModuleName(std::move(Name)), Path(std::move(Path)), ASTFile(std::move(ASTFile)), Signature(Signature) {} - ASTSourceDescriptor(Module &M); + ASTSourceDescriptor(const Module &M); std::string getModuleName() const; StringRef getPath() const { return Path; } StringRef getASTFile() const { return ASTFile; } ASTFileSignature getSignature() const { return Signature; } - Module *getModuleOrNull() const { return ClangModule; } + const Module *getModuleOrNull() const { return ClangModule; } }; diff --git a/clang/include/clang/Frontend/FrontendAction.h b/clang/include/clang/Frontend/FrontendAction.h index 039f6f247b6d8..6407728e39c6d 100644 --- a/clang/include/clang/Frontend/FrontendAction.h +++ b/clang/include/clang/Frontend/FrontendAction.h @@ -158,7 +158,7 @@ class FrontendAction { return *CurrentASTUnit; } - Module *getCurrentModule() const; + const Module *getCurrentModule() const; std::unique_ptr<ASTUnit> takeCurrentASTUnit() { return std::move(CurrentASTUnit); diff --git a/clang/include/clang/Frontend/MultiplexConsumer.h b/clang/include/clang/Frontend/MultiplexConsumer.h index 4ed0d86d3cdfb..bf8b50320e4e1 100644 --- a/clang/include/clang/Frontend/MultiplexConsumer.h +++ b/clang/include/clang/Frontend/MultiplexConsumer.h @@ -39,7 +39,7 @@ class MultiplexASTDeserializationListener : public ASTDeserializationListener { void SelectorRead(serialization::SelectorID iD, Selector Sel) override; void MacroDefinitionRead(serialization::PreprocessedEntityID, MacroDefinitionRecord *MD) override; - void ModuleRead(serialization::SubmoduleID ID, Module *Mod) override; + void ModuleRead(serialization::SubmoduleID ID, const Module *Mod) override; void ModuleImportRead(serialization::SubmoduleID ID, SourceLocation ImportLoc) override; diff --git a/clang/include/clang/Lex/ExternalPreprocessorSource.h b/clang/include/clang/Lex/ExternalPreprocessorSource.h index 6775841860373..51f93bc1b1242 100644 --- a/clang/include/clang/Lex/ExternalPreprocessorSource.h +++ b/clang/include/clang/Lex/ExternalPreprocessorSource.h @@ -39,7 +39,7 @@ class ExternalPreprocessorSource { virtual IdentifierInfo *GetIdentifier(unsigned ID) = 0; /// Map a module ID to a module. - virtual Module *getModule(unsigned ModuleID) = 0; + virtual const Module *getModule(unsigned ModuleID) = 0; }; } diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index 5ac63dddd4d4e..2e81f4f902c75 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -547,8 +547,8 @@ class HeaderSearch { /// \param M The module to which `File` belongs (this should usually be the /// SuggestedModule returned by LookupFile/LookupSubframeworkHeader) bool ShouldEnterIncludeFile(Preprocessor &PP, FileEntryRef File, - bool isImport, bool ModulesEnabled, Module *M, - bool &IsFirstIncludeOfFile); + bool isImport, bool ModulesEnabled, + const Module *M, bool &IsFirstIncludeOfFile); /// Return whether the specified file is a normal header, /// a system header, or a C++ friendly system header. @@ -622,7 +622,7 @@ class HeaderSearch { /// /// \returns The name of the module file that corresponds to this module, /// or an empty string if this module does not correspond to any module file. - std::string getCachedModuleFileName(Module *Module); + std::string getCachedModuleFileName(const Module *Module); /// Retrieve the name of the prebuilt module file that should be used /// to load a module with the given name. @@ -644,7 +644,7 @@ class HeaderSearch { /// /// \returns The name of the module file that corresponds to this module, /// or an empty string if this module does not correspond to any module file. - std::string getPrebuiltImplicitModuleFileName(Module *Module); + std::string getPrebuiltImplicitModuleFileName(const Module *Module); /// Retrieve the name of the (to-be-)cached module file that should /// be used to load a module with the given name. @@ -736,7 +736,7 @@ class HeaderSearch { /// Collect the set of all known, top-level modules. /// /// \param Modules Will be filled with the set of known, top-level modules. - void collectAllModules(SmallVectorImpl<Module *> &Modules); + void collectAllModules(SmallVectorImpl<const Module *> &Modules); /// Load all known, top-level system modules. void loadTopLevelSystemModules(); diff --git a/clang/include/clang/Lex/MacroInfo.h b/clang/include/clang/Lex/MacroInfo.h index 19a706216d509..162bf9777d227 100644 --- a/clang/include/clang/Lex/MacroInfo.h +++ b/clang/include/clang/Lex/MacroInfo.h @@ -521,7 +521,7 @@ class ModuleMacro : public llvm::FoldingSetNode { MacroInfo *Macro; /// The module that exports this macro. - Module *OwningModule; + const Module *OwningModule; /// The number of module macros that override this one. unsigned NumOverriddenBy = 0; @@ -529,8 +529,8 @@ class ModuleMacro : public llvm::FoldingSetNode { /// The number of modules whose macros are directly overridden by this one. unsigned NumOverrides; - ModuleMacro(Module *OwningModule, const IdentifierInfo *II, MacroInfo *Macro, - ArrayRef<ModuleMacro *> Overrides) + ModuleMacro(const Module *OwningModule, const IdentifierInfo *II, + MacroInfo *Macro, ArrayRef<ModuleMacro *> Overrides) : II(II), Macro(Macro), OwningModule(OwningModule), NumOverrides(Overrides.size()) { std::copy(Overrides.begin(), Overrides.end(), @@ -538,7 +538,7 @@ class ModuleMacro : public llvm::FoldingSetNode { } public: - static ModuleMacro *create(Preprocessor &PP, Module *OwningModule, + static ModuleMacro *create(Preprocessor &PP, const Module *OwningModule, const IdentifierInfo *II, MacroInfo *Macro, ArrayRef<ModuleMacro *> Overrides); @@ -546,7 +546,7 @@ class ModuleMacro : public llvm::FoldingSetNode { return Profile(ID, OwningModule, II); } - static void Profile(llvm::FoldingSetNodeID &ID, Module *OwningModule, + static void Profile(llvm::FoldingSetNodeID &ID, const Module *OwningModule, const IdentifierInfo *II) { ID.AddPointer(OwningModule); ID.AddPointer(II); @@ -556,7 +556,7 @@ class ModuleMacro : public llvm::FoldingSetNode { const IdentifierInfo *getName() const { return II; } /// Get the ID of the module that exports this macro. - Module *getOwningModule() const { return OwningModule; } + const Module *getOwningModule() const { return OwningModule; } /// Get definition for this exported #define, or nullptr if this /// represents a #undef. diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index 2e28ff6823cb2..4c5741412d600 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -105,7 +105,7 @@ class ModuleMap { llvm::DenseMap<const IdentifierInfo *, Module *> CachedModuleLoads; /// Shadow modules created while building this module map. - llvm::SmallVector<Module*, 2> ShadowModules; + llvm::SmallVector<const Module *, 2> ShadowModules; /// The number of modules we have created in total. unsigned NumCreatedModules = 0; @@ -182,7 +182,7 @@ class ModuleMap { } /// Whether this header is accessible from the specified module. - bool isAccessibleFrom(Module *M) const { + bool isAccessibleFrom(const Module *M) const { return !(getRole() & PrivateHeader) || (M && M->getTopLevelModule() == getModule()->getTopLevelModule()); } @@ -227,7 +227,7 @@ class ModuleMap { /// Modules from the same scope may not have the same name. unsigned CurrentModuleScopeID = 0; - llvm::DenseMap<Module *, unsigned> ModuleScopeIDs; + llvm::DenseMap<const Module *, unsigned> ModuleScopeIDs; /// The set of attributes that can be attached to a module. struct Attributes { @@ -300,7 +300,8 @@ class ModuleMap { /// \returns The resolved export declaration, which will have a NULL pointer /// if the export could not be resolved. Module::ExportDecl - resolveExport(Module *Mod, const Module::UnresolvedExportDecl &Unresolved, + resolveExport(const Module *Mod, + const Module::UnresolvedExportDecl &Unresolved, bool Complain) const; /// Resolve the given module id to an actual module. @@ -314,7 +315,8 @@ class ModuleMap { /// /// \returns The resolved module, or null if the module-id could not be /// resolved. - Module *resolveModuleId(const ModuleId &Id, Module *Mod, bool Complain) const; + Module *resolveModuleId(const ModuleId &Id, const Module *Mod, + bool Complain) const; /// Add an unresolved header to a module. /// @@ -337,7 +339,7 @@ class ModuleMap { /// be found in case M was, set it to true. False otherwise. /// \return The resolved file, if any. OptionalFileEntryRef - findHeader(Module *M, const Module::UnresolvedHeaderDirective &Header, + findHeader(const Module *M, const Module::UnresolvedHeaderDirective &Header, SmallVectorImpl<char> &RelativePathName, bool &NeedsFramework); /// Resolve the given header directive. @@ -382,8 +384,8 @@ class ModuleMap { return static_cast<bool>(findHeaderInUmbrellaDirs(File, IntermediateDirs)); } - Module *inferFrameworkModule(DirectoryEntryRef FrameworkDir, Attributes Attrs, - Module *Parent); + const Module *inferFrameworkModule(DirectoryEntryRef FrameworkDir, + Attributes Attrs, Module *Parent); public: /// Construct a new module map. @@ -418,7 +420,7 @@ class ModuleMap { bool isBuiltinHeader(FileEntryRef File); bool shouldImportRelativeToBuiltinIncludeDir(StringRef FileName, - Module *Module) const; + const Module *Module) const; /// Add a module map callback. void addModuleMapCallbacks(std::unique_ptr<ModuleMapCallbacks> Callback) { @@ -511,7 +513,7 @@ class ModuleMap { /// name lookup. /// /// \returns The named module, if known; otherwise, returns null. - Module *lookupModuleUnqualified(StringRef Name, Module *Context) const; + Module *lookupModuleUnqualified(StringRef Name, const Module *Context) const; /// Retrieve a module with the given name within the given context, /// using direct (qualified) name lookup. @@ -522,7 +524,7 @@ class ModuleMap { /// null, we will look for a top-level module. /// /// \returns The named submodule, if known; otherwose, returns null. - Module *lookupModuleQualified(StringRef Name, Module *Context) const; + Module *lookupModuleQualified(StringRef Name, const Module *Context) const; /// Find a new module or submodule, or create it if it does not already /// exist. @@ -585,13 +587,13 @@ cl... [truncated] |
| @llvm/pr-subscribers-debuginfo Author: David Stone (davidstone) ChangesPatch is 129.44 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/93493.diff 59 Files Affected:
diff --git a/clang/include/clang/APINotes/APINotesManager.h b/clang/include/clang/APINotes/APINotesManager.h index 18375c9e51a17..b559c24b322f2 100644 --- a/clang/include/clang/APINotes/APINotesManager.h +++ b/clang/include/clang/APINotes/APINotesManager.h @@ -145,7 +145,7 @@ class APINotesManager { /// /// \returns a vector of FileEntry where APINotes files are. llvm::SmallVector<FileEntryRef, 2> - getCurrentModuleAPINotes(Module *M, bool LookInModule, + getCurrentModuleAPINotes(const Module *M, bool LookInModule, ArrayRef<std::string> SearchPaths); /// Load Compiled API notes for current module. diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index a1d1d1c51cd41..70d131c12e073 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -462,7 +462,7 @@ class ASTContext : public RefCountedBase<ASTContext> { void resolve(ASTContext &Ctx); }; - llvm::DenseMap<Module*, PerModuleInitializers*> ModuleInitializers; + llvm::DenseMap<const Module *, PerModuleInitializers *> ModuleInitializers; /// This is the top-level (C++20) Named module we are building. Module *CurrentCXXNamedModule = nullptr; @@ -1060,12 +1060,12 @@ class ASTContext : public RefCountedBase<ASTContext> { /// for a module. This will typically be a global variable (with internal /// linkage) that runs module initializers, such as the iostream initializer, /// or an ImportDecl nominating another module that has initializers. - void addModuleInitializer(Module *M, Decl *Init); + void addModuleInitializer(const Module *M, Decl *Init); - void addLazyModuleInitializers(Module *M, ArrayRef<GlobalDeclID> IDs); + void addLazyModuleInitializers(const Module *M, ArrayRef<GlobalDeclID> IDs); /// Get the initializations to perform when importing a module, if any. - ArrayRef<Decl*> getModuleInitializers(Module *M); + ArrayRef<Decl *> getModuleInitializers(const Module *M); /// Set the (C++20) module we are building. void setCurrentNamedModule(Module *M); diff --git a/clang/include/clang/AST/ASTMutationListener.h b/clang/include/clang/AST/ASTMutationListener.h index 2c4ec2ce67f36..fc476f531815c 100644 --- a/clang/include/clang/AST/ASTMutationListener.h +++ b/clang/include/clang/AST/ASTMutationListener.h @@ -139,7 +139,7 @@ class ASTMutationListener { /// \param D The definition that was previously not visible. /// \param M The containing module in which the definition was made visible, /// if any. - virtual void RedefinedHiddenDefinition(const NamedDecl *D, Module *M) {} + virtual void RedefinedHiddenDefinition(const NamedDecl *D, const Module *M) {} /// An attribute was added to a RecordDecl /// diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h index e43e812cd9455..e7dac0c67ff3d 100644 --- a/clang/include/clang/AST/DeclBase.h +++ b/clang/include/clang/AST/DeclBase.h @@ -817,11 +817,11 @@ class alignas(8) Decl { "owned local decl but no local module storage"); return reinterpret_cast<Module *const *>(this)[-1]; } - void setLocalOwningModule(Module *M) { + void setLocalOwningModule(const Module *M) { assert(!isFromASTFile() && hasOwningModule() && hasLocalOwningModuleStorage() && "should not have a cached owning module"); - reinterpret_cast<Module **>(this)[-1] = M; + reinterpret_cast<const Module **>(this)[-1] = M; } /// Is this declaration owned by some module? @@ -839,7 +839,7 @@ class alignas(8) Decl { /// /// \param IgnoreLinkage Ignore the linkage of the entity; assume that /// all declarations in a global module fragment are unowned. - Module *getOwningModuleForLinkage(bool IgnoreLinkage = false) const; + const Module *getOwningModuleForLinkage(bool IgnoreLinkage = false) const; /// Determine whether this declaration is definitely visible to name lookup, /// independent of whether the owning module is visible. diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h index 2d62d05cd9190..d85e59bd5e9bb 100644 --- a/clang/include/clang/Basic/Module.h +++ b/clang/include/clang/Basic/Module.h @@ -296,7 +296,7 @@ class alignas(8) Module { SmallVector<Requirement, 2> Requirements; /// A module with the same name that shadows this module. - Module *ShadowingModule = nullptr; + const Module *ShadowingModule = nullptr; /// Whether this module has declared itself unimportable, either because /// it's missing a requirement from \p Requirements or because it's been @@ -403,7 +403,7 @@ class alignas(8) Module { /// The set of top-level modules that affected the compilation of this module, /// but were not imported. - llvm::SmallSetVector<Module *, 2> AffectingClangModules; + llvm::SmallSetVector<const Module *, 2> AffectingClangModules; /// Describes an exported module. /// @@ -433,7 +433,7 @@ class alignas(8) Module { SmallVector<UnresolvedExportDecl, 2> UnresolvedExports; /// The directly used modules. - SmallVector<Module *, 2> DirectUses; + SmallVector<const Module *, 2> DirectUses; /// The set of use declarations that have yet to be resolved. SmallVector<ModuleId, 2> UnresolvedDirectUses; @@ -487,7 +487,7 @@ class alignas(8) Module { /// A conflict between two modules. struct Conflict { /// The module that this module conflicts with. - Module *Other; + const Module *Other; /// The message provided to the user when there is a conflict. std::string Message; @@ -519,7 +519,7 @@ class alignas(8) Module { /// \param ShadowingModule If this module is unimportable because it is /// shadowed, this parameter will be set to the shadowing module. bool isUnimportable(const LangOptions &LangOpts, const TargetInfo &Target, - Requirement &Req, Module *&ShadowingModule) const; + Requirement &Req, const Module *&ShadowingModule) const; /// Determine whether this module can be built in this compilation. bool isForBuilding(const LangOptions &LangOpts) const; @@ -545,11 +545,9 @@ class alignas(8) Module { /// /// \param ShadowingModule If this module is unavailable because it is /// shadowed, this parameter will be set to the shadowing module. - bool isAvailable(const LangOptions &LangOpts, - const TargetInfo &Target, - Requirement &Req, - UnresolvedHeaderDirective &MissingHeader, - Module *&ShadowingModule) const; + bool isAvailable(const LangOptions &LangOpts, const TargetInfo &Target, + Requirement &Req, UnresolvedHeaderDirective &MissingHeader, + const Module *&ShadowingModule) const; /// Determine whether this module is a submodule. bool isSubModule() const { return Parent != nullptr; } @@ -755,13 +753,13 @@ class alignas(8) Module { /// one. /// /// \returns The GMF sub-module if found, or NULL otherwise. - Module *getGlobalModuleFragment() const; + const Module *getGlobalModuleFragment() const; /// Get the Private Module Fragment (sub-module) for this module, it there is /// one. /// /// \returns The PMF sub-module if found, or NULL otherwise. - Module *getPrivateModuleFragment() const; + const Module *getPrivateModuleFragment() const; /// Determine whether the specified module would be visible to /// a lookup at the end of this module. @@ -845,20 +843,22 @@ class VisibleModuleSet { /// A callback to call when a module is made visible (directly or /// indirectly) by a call to \ref setVisible. - using VisibleCallback = llvm::function_ref<void(Module *M)>; + using VisibleCallback = llvm::function_ref<void(const Module *M)>; /// A callback to call when a module conflict is found. \p Path /// consists of a sequence of modules from the conflicting module to the one /// made visible, where each was exported by the next. using ConflictCallback = - llvm::function_ref<void(ArrayRef<Module *> Path, Module *Conflict, - StringRef Message)>; + llvm::function_ref<void(ArrayRef<const Module *> Path, + const Module *Conflict, StringRef Message)>; /// Make a specific module visible. - void setVisible(Module *M, SourceLocation Loc, - VisibleCallback Vis = [](Module *) {}, - ConflictCallback Cb = [](ArrayRef<Module *>, Module *, - StringRef) {}); + void setVisible( + const Module *M, SourceLocation Loc, + VisibleCallback Vis = [](const Module *) {}, + ConflictCallback Cb = [](ArrayRef<const Module *>, const Module *, + StringRef) {}); + private: /// Import locations for each visible module. Indexed by the module's /// VisibilityID. @@ -876,7 +876,7 @@ class ASTSourceDescriptor { StringRef Path; StringRef ASTFile; ASTFileSignature Signature; - Module *ClangModule = nullptr; + const Module *ClangModule = nullptr; public: ASTSourceDescriptor() = default; @@ -884,13 +884,13 @@ class ASTSourceDescriptor { ASTFileSignature Signature) : PCHModuleName(std::move(Name)), Path(std::move(Path)), ASTFile(std::move(ASTFile)), Signature(Signature) {} - ASTSourceDescriptor(Module &M); + ASTSourceDescriptor(const Module &M); std::string getModuleName() const; StringRef getPath() const { return Path; } StringRef getASTFile() const { return ASTFile; } ASTFileSignature getSignature() const { return Signature; } - Module *getModuleOrNull() const { return ClangModule; } + const Module *getModuleOrNull() const { return ClangModule; } }; diff --git a/clang/include/clang/Frontend/FrontendAction.h b/clang/include/clang/Frontend/FrontendAction.h index 039f6f247b6d8..6407728e39c6d 100644 --- a/clang/include/clang/Frontend/FrontendAction.h +++ b/clang/include/clang/Frontend/FrontendAction.h @@ -158,7 +158,7 @@ class FrontendAction { return *CurrentASTUnit; } - Module *getCurrentModule() const; + const Module *getCurrentModule() const; std::unique_ptr<ASTUnit> takeCurrentASTUnit() { return std::move(CurrentASTUnit); diff --git a/clang/include/clang/Frontend/MultiplexConsumer.h b/clang/include/clang/Frontend/MultiplexConsumer.h index 4ed0d86d3cdfb..bf8b50320e4e1 100644 --- a/clang/include/clang/Frontend/MultiplexConsumer.h +++ b/clang/include/clang/Frontend/MultiplexConsumer.h @@ -39,7 +39,7 @@ class MultiplexASTDeserializationListener : public ASTDeserializationListener { void SelectorRead(serialization::SelectorID iD, Selector Sel) override; void MacroDefinitionRead(serialization::PreprocessedEntityID, MacroDefinitionRecord *MD) override; - void ModuleRead(serialization::SubmoduleID ID, Module *Mod) override; + void ModuleRead(serialization::SubmoduleID ID, const Module *Mod) override; void ModuleImportRead(serialization::SubmoduleID ID, SourceLocation ImportLoc) override; diff --git a/clang/include/clang/Lex/ExternalPreprocessorSource.h b/clang/include/clang/Lex/ExternalPreprocessorSource.h index 6775841860373..51f93bc1b1242 100644 --- a/clang/include/clang/Lex/ExternalPreprocessorSource.h +++ b/clang/include/clang/Lex/ExternalPreprocessorSource.h @@ -39,7 +39,7 @@ class ExternalPreprocessorSource { virtual IdentifierInfo *GetIdentifier(unsigned ID) = 0; /// Map a module ID to a module. - virtual Module *getModule(unsigned ModuleID) = 0; + virtual const Module *getModule(unsigned ModuleID) = 0; }; } diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h index 5ac63dddd4d4e..2e81f4f902c75 100644 --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -547,8 +547,8 @@ class HeaderSearch { /// \param M The module to which `File` belongs (this should usually be the /// SuggestedModule returned by LookupFile/LookupSubframeworkHeader) bool ShouldEnterIncludeFile(Preprocessor &PP, FileEntryRef File, - bool isImport, bool ModulesEnabled, Module *M, - bool &IsFirstIncludeOfFile); + bool isImport, bool ModulesEnabled, + const Module *M, bool &IsFirstIncludeOfFile); /// Return whether the specified file is a normal header, /// a system header, or a C++ friendly system header. @@ -622,7 +622,7 @@ class HeaderSearch { /// /// \returns The name of the module file that corresponds to this module, /// or an empty string if this module does not correspond to any module file. - std::string getCachedModuleFileName(Module *Module); + std::string getCachedModuleFileName(const Module *Module); /// Retrieve the name of the prebuilt module file that should be used /// to load a module with the given name. @@ -644,7 +644,7 @@ class HeaderSearch { /// /// \returns The name of the module file that corresponds to this module, /// or an empty string if this module does not correspond to any module file. - std::string getPrebuiltImplicitModuleFileName(Module *Module); + std::string getPrebuiltImplicitModuleFileName(const Module *Module); /// Retrieve the name of the (to-be-)cached module file that should /// be used to load a module with the given name. @@ -736,7 +736,7 @@ class HeaderSearch { /// Collect the set of all known, top-level modules. /// /// \param Modules Will be filled with the set of known, top-level modules. - void collectAllModules(SmallVectorImpl<Module *> &Modules); + void collectAllModules(SmallVectorImpl<const Module *> &Modules); /// Load all known, top-level system modules. void loadTopLevelSystemModules(); diff --git a/clang/include/clang/Lex/MacroInfo.h b/clang/include/clang/Lex/MacroInfo.h index 19a706216d509..162bf9777d227 100644 --- a/clang/include/clang/Lex/MacroInfo.h +++ b/clang/include/clang/Lex/MacroInfo.h @@ -521,7 +521,7 @@ class ModuleMacro : public llvm::FoldingSetNode { MacroInfo *Macro; /// The module that exports this macro. - Module *OwningModule; + const Module *OwningModule; /// The number of module macros that override this one. unsigned NumOverriddenBy = 0; @@ -529,8 +529,8 @@ class ModuleMacro : public llvm::FoldingSetNode { /// The number of modules whose macros are directly overridden by this one. unsigned NumOverrides; - ModuleMacro(Module *OwningModule, const IdentifierInfo *II, MacroInfo *Macro, - ArrayRef<ModuleMacro *> Overrides) + ModuleMacro(const Module *OwningModule, const IdentifierInfo *II, + MacroInfo *Macro, ArrayRef<ModuleMacro *> Overrides) : II(II), Macro(Macro), OwningModule(OwningModule), NumOverrides(Overrides.size()) { std::copy(Overrides.begin(), Overrides.end(), @@ -538,7 +538,7 @@ class ModuleMacro : public llvm::FoldingSetNode { } public: - static ModuleMacro *create(Preprocessor &PP, Module *OwningModule, + static ModuleMacro *create(Preprocessor &PP, const Module *OwningModule, const IdentifierInfo *II, MacroInfo *Macro, ArrayRef<ModuleMacro *> Overrides); @@ -546,7 +546,7 @@ class ModuleMacro : public llvm::FoldingSetNode { return Profile(ID, OwningModule, II); } - static void Profile(llvm::FoldingSetNodeID &ID, Module *OwningModule, + static void Profile(llvm::FoldingSetNodeID &ID, const Module *OwningModule, const IdentifierInfo *II) { ID.AddPointer(OwningModule); ID.AddPointer(II); @@ -556,7 +556,7 @@ class ModuleMacro : public llvm::FoldingSetNode { const IdentifierInfo *getName() const { return II; } /// Get the ID of the module that exports this macro. - Module *getOwningModule() const { return OwningModule; } + const Module *getOwningModule() const { return OwningModule; } /// Get definition for this exported #define, or nullptr if this /// represents a #undef. diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index 2e28ff6823cb2..4c5741412d600 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -105,7 +105,7 @@ class ModuleMap { llvm::DenseMap<const IdentifierInfo *, Module *> CachedModuleLoads; /// Shadow modules created while building this module map. - llvm::SmallVector<Module*, 2> ShadowModules; + llvm::SmallVector<const Module *, 2> ShadowModules; /// The number of modules we have created in total. unsigned NumCreatedModules = 0; @@ -182,7 +182,7 @@ class ModuleMap { } /// Whether this header is accessible from the specified module. - bool isAccessibleFrom(Module *M) const { + bool isAccessibleFrom(const Module *M) const { return !(getRole() & PrivateHeader) || (M && M->getTopLevelModule() == getModule()->getTopLevelModule()); } @@ -227,7 +227,7 @@ class ModuleMap { /// Modules from the same scope may not have the same name. unsigned CurrentModuleScopeID = 0; - llvm::DenseMap<Module *, unsigned> ModuleScopeIDs; + llvm::DenseMap<const Module *, unsigned> ModuleScopeIDs; /// The set of attributes that can be attached to a module. struct Attributes { @@ -300,7 +300,8 @@ class ModuleMap { /// \returns The resolved export declaration, which will have a NULL pointer /// if the export could not be resolved. Module::ExportDecl - resolveExport(Module *Mod, const Module::UnresolvedExportDecl &Unresolved, + resolveExport(const Module *Mod, + const Module::UnresolvedExportDecl &Unresolved, bool Complain) const; /// Resolve the given module id to an actual module. @@ -314,7 +315,8 @@ class ModuleMap { /// /// \returns The resolved module, or null if the module-id could not be /// resolved. - Module *resolveModuleId(const ModuleId &Id, Module *Mod, bool Complain) const; + Module *resolveModuleId(const ModuleId &Id, const Module *Mod, + bool Complain) const; /// Add an unresolved header to a module. /// @@ -337,7 +339,7 @@ class ModuleMap { /// be found in case M was, set it to true. False otherwise. /// \return The resolved file, if any. OptionalFileEntryRef - findHeader(Module *M, const Module::UnresolvedHeaderDirective &Header, + findHeader(const Module *M, const Module::UnresolvedHeaderDirective &Header, SmallVectorImpl<char> &RelativePathName, bool &NeedsFramework); /// Resolve the given header directive. @@ -382,8 +384,8 @@ class ModuleMap { return static_cast<bool>(findHeaderInUmbrellaDirs(File, IntermediateDirs)); } - Module *inferFrameworkModule(DirectoryEntryRef FrameworkDir, Attributes Attrs, - Module *Parent); + const Module *inferFrameworkModule(DirectoryEntryRef FrameworkDir, + Attributes Attrs, Module *Parent); public: /// Construct a new module map. @@ -418,7 +420,7 @@ class ModuleMap { bool isBuiltinHeader(FileEntryRef File); bool shouldImportRelativeToBuiltinIncludeDir(StringRef FileName, - Module *Module) const; + const Module *Module) const; /// Add a module map callback. void addModuleMapCallbacks(std::unique_ptr<ModuleMapCallbacks> Callback) { @@ -511,7 +513,7 @@ class ModuleMap { /// name lookup. /// /// \returns The named module, if known; otherwise, returns null. - Module *lookupModuleUnqualified(StringRef Name, Module *Context) const; + Module *lookupModuleUnqualified(StringRef Name, const Module *Context) const; /// Retrieve a module with the given name within the given context, /// using direct (qualified) name lookup. @@ -522,7 +524,7 @@ class ModuleMap { /// null, we will look for a top-level module. /// /// \returns The named submodule, if known; otherwose, returns null. - Module *lookupModuleQualified(StringRef Name, Module *Context) const; + Module *lookupModuleQualified(StringRef Name, const Module *Context) const; /// Find a new module or submodule, or create it if it does not already /// exist. @@ -585,13 +587,13 @@ cl... [truncated] |
| Can you make sure that at every place this PR touches |
I'm not quite sure I understand the question. This PR doesn't add any |
To add a little more flavor to my response, this PR is a precursor to a larger refactoring of |
| Sounds like you have a plan after this PR, which is good. I was worried that you're just applying |
| Yes. Is there anything else you want to see in this PR before it can be merged? (I don't have merge permissions). This is the type of PR likely to get lots of conflicts if it stays open for long, so I'd like to get it wrapped up as fast as possible. |
The question is that it may be fine to be |
AaronBallman left a comment
AaronBallman left a comment There was a problem hiding this comment.
In general, I'm in favor of improving const-correctness like this. That said, I share the concerns of other reviewers that we should be sure we're adding const-correctness where it makes sense to do so. e.g., functions named getSomething() that return a pointer should probably be returning a const pointer. But for a function named setSomething() taking a pointer, it might not make sense to take a const pointer even if that's permissible currently.
| I'll admit, I'm surprised this PR has any push-back at all since this feels like just applying a very generally accepted principle. Things should be const by default -- you need a reason to declare something as not const, you should never need a reason to declare something as const. It's perfectly normal for a function named Right now, a lot of the clang code is designed as "Here is a whole bunch of state that huge chunks of the program have access to", which means that understanding how any part works requires understanding how everything works -- you cannot reason about anything in isolation. This is a problem. The first step in fixing that is constraining who can modify the object. This commit is one of the sub-steps in that step. The next major commit I am working on tries to better encapsulate Once we have that, the understanding how the state of a Module changes over time requires understanding a significantly smaller part of the codebase, which will then allow another follow-up commit that changes |
| You don't have to convince me that being const-correct is a good thing, I'm already in full support of that. :-) The push back isn't "no, don't do this", it's "we're not const correct in a lot of places and so adding const correctness to a part of the compiler without considering the bigger picture means increasing the chances of needing to add |
| My philosophy on problems like that is that the first step to getting out of a hole is to stop digging the hole. We aren't going to make all of clang const-correct overnight, right? My goal here is starting with this one type: |
| I could agree on being const correct here, but mostly by removing const, not adding more, in the general case. The problem here is that this is such a big code base, and where some times parameters can be passed down from function to function for a very long depth, that this can make some seemingly small local changes propagate into a lot of code churn. |
Yup! I think we're in agreement with the long-term goal and how we get there. The devil is in the details (as always). For example, one thing we do all over the place in Clang is
Okay, that's good to know, thank you!
Errr, not certain I agree with this -- that basically is "admit defeat and stop aiming for const correctness." |
Well, I am saying, add const to places we are pretty sure we will never change, and leave const out when in doubt. |
That's the opposite of my view. Mutation needs to be justified. "What if we need it later" can be used to justify anything, and if we do need it later then we change the code then. Until that point, readers can see |
I agree that works well for small code bases, but in LLVM some of these changes lead to so much churn, I start to question their benefit. Speaking from a been there, done that POV. |
| One suggestion, if you are going to go about this problem systematically, try to tackle the hardest problems first. For example: Try to make |
| I think I should post here an example from our codebase that made me think twice about this PR. First, there's a llvm-project/clang/include/clang/Sema/Template.h Lines 76 to 79 in 42b4be6 As you can see, actual argument list is defined as an ArrayRef, which is a view type that propagates constness to its elements. Then, there's a llvm-project/clang/include/clang/Sema/Template.h Lines 196 to 204 in 42b4be6 const_cast here is a relatively recent addition, and I checked out with @erichkeane that the use case (pack expansion) is legit. According to the approach you're suggesting, the one who wrote this const_cast should instead refactor the MLTAL to use llvm::MutableArrayRef. I tried, pretty hard, but ultimately failed. The change turned out to be touching every corner of our template engine, with no end in sight. While you're claiming that annotating things with |
Take a guess who that person was 😁
Oh well, sorry I wasn't around to advise against it. |
Right, yeah. Unfortunately const-correctness in Clang is really bad, and not really something we can fix well piecemeal without a lot of churn/issues. Our current approach is pretty pervasive to breaking const-correctness, so any iterative/piecemeal appraoch ends up having to const-cast all over the place. I am pretty sure a few dozen folks have attempted over the last few years, each ended up more annoyed than the last :) That said, this patch Is something I think is a good direction. However, I fear(it seems?) it doesn't use any level of holistic approach to determining which functions need to mutate other than "they don't mutate today". Which is, IMO, a worse level of const-correctness to be in. We need to identify which functions/uses are conceptually modifying (see the |
No worries, I was eliminating |
…can be accessed mutably.
| BTW, for the series patches, it may be better to use stacked PR (we use stacked PR by the method in https://llvm.org/docs/GitHub.html) if you have following patches (and it looks like you have a lot such patches). Also, as a downstream vendor, I hope we can land the (large, NFC) patch series together. Since it shows some pains in backporting such patch series. |
AaronBallman left a comment
AaronBallman left a comment There was a problem hiding this comment.
I looked through the review and I think this is generally an improvement. We may find a need to relax some const later, but I did not spot any obvious concerns.
That said, I'd like @ChuanqiXu9 to give the final sign-off on this as modules code owner.
ChuanqiXu9 left a comment
ChuanqiXu9 left a comment There was a problem hiding this comment.
I feel good with this too.
But if later you have similar multiple consecutive large NFC patches like this, I'll recommend you send the PR as stacked PR and then we can land the continuously. It'll help the downstream project slightly.
| @davidstone Do you plan to get back to this PR? |
7 participants
No description provided.