r212387 - Use non-intrusive refcounting for LangOptions

[Alp Toker]

On 06/07/2014 21:38, Nico Weber wrote:
> On Sat, Jul 5, 2014 at 10:26 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     Author: alp
>     Date: Sun Jul  6 00:26:07 2014
>     New Revision: 212387
>
>     URL: http://llvm.org/viewvc/llvm-project?rev=212387&view=rev
>     Log:
>     Use non-intrusive refcounting for LangOptions
>
>     This type is only refcounted in a couple of places so making
>     ownership explicit
>     improves clarity.
>
>     Modified:
>         cfe/trunk/include/clang/Basic/LangOptions.h
>         cfe/trunk/include/clang/Frontend/ASTUnit.h
>         cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>         cfe/trunk/lib/Frontend/ASTUnit.cpp
>
>     Modified: cfe/trunk/include/clang/Basic/LangOptions.h
>     URL:
>     http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/LangOptions.h?rev=212387&r1=212386&r2=212387&view=diff
>     ==============================================================================
>     --- cfe/trunk/include/clang/Basic/LangOptions.h (original)
>     +++ cfe/trunk/include/clang/Basic/LangOptions.h Sun Jul  6
>     00:26:07 2014
>     @@ -19,7 +19,6 @@
>      #include "clang/Basic/LLVM.h"
>      #include "clang/Basic/ObjCRuntime.h"
>      #include "clang/Basic/Visibility.h"
>     -#include "llvm/ADT/IntrusiveRefCntPtr.h"
>      #include <string>
>
>      namespace clang {
>     @@ -53,7 +52,7 @@ protected:
>
>      /// \brief Keeps track of the various options that can be
>      /// enabled, which controls the dialect of C or C++ that is accepted.
>     -class LangOptions : public RefCountedBase<LangOptions>, public
>     LangOptionsBase {
>     +class LangOptions : public LangOptionsBase {
>      public:
>        typedef clang::Visibility Visibility;
>
>
>     Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h
>     URL:
>     http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=212387&r1=212386&r2=212387&view=diff
>     ==============================================================================
>     --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original)
>     +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Sun Jul  6 00:26:07
>     2014
>     @@ -83,7 +83,7 @@ public:
>        };
>
>      private:
>     -  IntrusiveRefCntPtr<LangOptions>         LangOpts;
>     +  std::shared_ptr<LangOptions>            LangOpts;
>        IntrusiveRefCntPtr<DiagnosticsEngine> Diagnostics;
>        IntrusiveRefCntPtr<FileManager>         FileMgr;
>        IntrusiveRefCntPtr<SourceManager> SourceMgr;
>
>     Modified: cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>     URL:
>     http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInvocation.h?rev=212387&r1=212386&r2=212387&view=diff
>     ==============================================================================
>     --- cfe/trunk/include/clang/Frontend/CompilerInvocation.h (original)
>     +++ cfe/trunk/include/clang/Frontend/CompilerInvocation.h Sun Jul
>      6 00:26:07 2014
>     @@ -52,9 +52,9 @@ bool ParseDiagnosticArgs(DiagnosticOptio
>      class CompilerInvocationBase : public
>     RefCountedBase<CompilerInvocation> {
>        void operator=(const CompilerInvocationBase &)
>     LLVM_DELETED_FUNCTION;
>
>     -protected:
>     +public:
>
>
> Why make the fields public?

This makes the refcounted fields public to enable shared ownership in 
the few places it's necessary.

I chose this approach in preference to exposing the refcount via the 
existing accessors because we want to restrict most of clang to reading 
from the structure without adding a reference.

We could add new get*Reference() variants for each of the existing 
accesors but I'm not convinced it'd add any value, but would make it 
less clear which objects are holding the current reference and where 
ownership is being transferred from.

Alp.




>        /// Options controlling the language variant.
>     -  IntrusiveRefCntPtr<LangOptions> LangOpts;
>     +  std::shared_ptr<LangOptions> LangOpts;
>
>        /// Options controlling the target.
>        IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>     @@ -68,7 +68,6 @@ protected:
>        /// Options controlling the preprocessor (aside from \#include
>     handling).
>        IntrusiveRefCntPtr<PreprocessorOptions> PreprocessorOpts;
>
>     -public:
>        CompilerInvocationBase();
>        ~CompilerInvocationBase();
>
>
>     Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
>     URL:
>     http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=212387&r1=212386&r2=212387&view=diff
>     ==============================================================================
>     --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
>     +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Sun Jul  6 00:26:07 2014
>     @@ -1087,7 +1087,7 @@ bool ASTUnit::Parse(llvm::MemoryBuffer *
>               "IR inputs not support here!");
>
>        // Configure the various subsystems.
>     -  LangOpts = &Clang->getLangOpts();
>     +  LangOpts = Clang->getInvocation().LangOpts;
>        FileSystemOpts = Clang->getFileSystemOpts();
>        IntrusiveRefCntPtr<vfs::FileSystem> VFS =
>      createVFSFromCompilerInvocation(Clang->getInvocation(),
>     getDiagnostics());
>     @@ -1709,7 +1709,7 @@ void ASTUnit::transferASTDataFromCompile
>        // Steal the created target, context, and preprocessor if they
>     have been
>        // created.
>        assert(CI.hasInvocation() && "missing invocation");
>     -  LangOpts = CI.getInvocation().getLangOpts();
>     +  LangOpts = CI.getInvocation().LangOpts;
>        TheSema.reset(CI.takeSema());
>        Consumer.reset(CI.takeASTConsumer());
>        if (CI.hasASTContext())
>
>
>     _______________________________________________
>     cfe-commits mailing list
>     cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>     http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>

-- 
http://www.nuanti.com
the browser experts