r212388 - Use non-intrusive refcounting for TargetOptions
David Blaikie
dblaikie at gmail.com
Wed Jul 16 10:40:52 PDT 2014
On Thu, Jul 10, 2014 at 6:08 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 07/07/2014 23:55, David Blaikie wrote:
>>
>> On Sat, Jul 5, 2014 at 10:26 PM, Alp Toker <alp at nuanti.com> wrote:
>>>
>>> Author: alp
>>> Date: Sun Jul 6 00:26:44 2014
>>> New Revision: 212388
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=212388&view=rev
>>> Log:
>>> Use non-intrusive refcounting for TargetOptions
>>
>> Awesome - thanks! (love incremental improvement, especially around
>> ownership semantics)
>>
>>> Modified:
>>> cfe/trunk/include/clang/Basic/TargetInfo.h
>>> cfe/trunk/include/clang/Basic/TargetOptions.h
>>> cfe/trunk/include/clang/Frontend/ASTUnit.h
>>> cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>>> cfe/trunk/lib/Basic/Targets.cpp
>>> cfe/trunk/lib/Frontend/ASTUnit.cpp
>>> cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp
>>> cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>> cfe/trunk/unittests/Basic/SourceManagerTest.cpp
>>> cfe/trunk/unittests/Lex/LexerTest.cpp
>>> cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
>>> cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/TargetInfo.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/TargetInfo.h?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/TargetInfo.h (original)
>>> +++ cfe/trunk/include/clang/Basic/TargetInfo.h Sun Jul 6 00:26:44 2014
>>> @@ -47,7 +47,7 @@ namespace Builtin { struct Info; }
>>> /// \brief Exposes information about the current target.
>>> ///
>>> class TargetInfo : public RefCountedBase<TargetInfo> {
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> llvm::Triple Triple;
>>> protected:
>>> // Target values set by the ctor of the actual target implementation.
>>> Default
>>> @@ -94,8 +94,9 @@ public:
>>> /// \param Opts - The options to use to initialize the target. The
>>> target may
>>> /// modify the options to canonicalize the target feature information
>>> to match
>>> /// what the backend expects.
>>> - static TargetInfo* CreateTargetInfo(DiagnosticsEngine &Diags,
>>> - TargetOptions *Opts);
>>> + static TargetInfo *
>>> + CreateTargetInfo(DiagnosticsEngine &Diags,
>>> + const std::shared_ptr<TargetOptions> &Opts);
>>>
>>> virtual ~TargetInfo();
>>>
>>> @@ -105,10 +106,6 @@ public:
>>> return *TargetOpts;
>>> }
>>>
>>> - void setTargetOpts(TargetOptions *TargetOpts) {
>>> - this->TargetOpts = TargetOpts;
>>> - }
>>> -
>>> ///===---- Target Data Type Query Methods
>>> -------------------------------===//
>>> enum IntType {
>>> NoInt = 0,
>>>
>>> Modified: cfe/trunk/include/clang/Basic/TargetOptions.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/TargetOptions.h?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Basic/TargetOptions.h (original)
>>> +++ cfe/trunk/include/clang/Basic/TargetOptions.h Sun Jul 6 00:26:44
>>> 2014
>>> @@ -15,15 +15,13 @@
>>> #ifndef LLVM_CLANG_FRONTEND_TARGETOPTIONS_H
>>> #define LLVM_CLANG_FRONTEND_TARGETOPTIONS_H
>>>
>>> -#include "clang/Basic/LLVM.h"
>>> -#include "llvm/ADT/IntrusiveRefCntPtr.h"
>>> #include <string>
>>> #include <vector>
>>>
>>> namespace clang {
>>>
>>> /// \brief Options for controlling the target.
>>> -class TargetOptions : public RefCountedBase<TargetOptions> {
>>> +class TargetOptions {
>>> public:
>>> /// If given, the name of the target triple to compile for. If not
>>> given the
>>> /// target will be selected to match the host.
>>>
>>> Modified: cfe/trunk/include/clang/Frontend/ASTUnit.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/ASTUnit.h?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Frontend/ASTUnit.h (original)
>>> +++ cfe/trunk/include/clang/Frontend/ASTUnit.h Sun Jul 6 00:26:44 2014
>>> @@ -91,7 +91,7 @@ private:
>>> IntrusiveRefCntPtr<TargetInfo> Target;
>>> IntrusiveRefCntPtr<Preprocessor> PP;
>>> IntrusiveRefCntPtr<ASTContext> Ctx;
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> IntrusiveRefCntPtr<HeaderSearchOptions> HSOpts;
>>> IntrusiveRefCntPtr<ASTReader> Reader;
>>> bool HadModuleLoaderFatalFailure;
>>>
>>> Modified: cfe/trunk/include/clang/Frontend/CompilerInvocation.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/CompilerInvocation.h?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Frontend/CompilerInvocation.h (original)
>>> +++ cfe/trunk/include/clang/Frontend/CompilerInvocation.h Sun Jul 6
>>> 00:26:44 2014
>>> @@ -57,7 +57,7 @@ public:
>>> std::shared_ptr<LangOptions> LangOpts;
>>>
>>> /// Options controlling the target.
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>>
>>> /// Options controlling the diagnostic engine.
>>> IntrusiveRefCntPtr<DiagnosticOptions> DiagnosticOpts;
>>>
>>> Modified: cfe/trunk/lib/Basic/Targets.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Basic/Targets.cpp (original)
>>> +++ cfe/trunk/lib/Basic/Targets.cpp Sun Jul 6 00:26:44 2014
>>> @@ -6388,8 +6388,9 @@ static TargetInfo *AllocateTarget(const
>>>
>>> /// CreateTargetInfo - Return the target info object for the specified
>>> target
>>> /// triple.
>>> -TargetInfo *TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
>>> - TargetOptions *Opts) {
>>> +TargetInfo *
>>> +TargetInfo::CreateTargetInfo(DiagnosticsEngine &Diags,
>>> + const std::shared_ptr<TargetOptions> &Opts)
>>> {
>>
>> Pass std::shared_ptr by value here?
>
>
> That would mean having to refactor the function body..
>
>
>>
>>> llvm::Triple Triple(Opts->Triple);
>>>
>>> // Construct the target
>>> @@ -6398,7 +6399,7 @@ TargetInfo *TargetInfo::CreateTargetInfo
>>> Diags.Report(diag::err_target_unknown_triple) << Triple.str();
>>> return nullptr;
>>> }
>>> - Target->setTargetOpts(Opts);
>>> + Target->TargetOpts = Opts;
>>
>> and std::move it here?
>
>
> .. because std::moving it here would cause Opts to become empty, and Opts is
> currently used directly throughout the function.
>
> And we *really* don't want to be refactoring this code as a drive-by
> cleanup, it's full of surprises.
>
>
>> That way if the parameter happens to be a
>> temporary, we'll avoid an extra reference count increment/decrement.
>
> On the other hand, changing it to pass-by-value would incur an additional
> refcount (in the early return case), whereas the current code on ToT never
> incurs an extra refcount. It's not clear cut, and n balance I believe my
> commit has got this right.
Though the early return is an error case, the fact that (as you point
out) no callers are passing xvalues means this has no immediate
benefit and a (small/error case) disadvantage, so I'd say you're right
here. Thanks for pointing out the details.
It is an annoying case once there's conditional copy - having to write
the same function twice to be optimal (in cases where there are
callers that pass temporaries, or might be in the future). Not sure
what the right answer is there, though it comes up surprisingly
infrequently in my experience, so I won't worry too much.
> Tangentially, I don't think std::move() is ever a good idea if it can be
> avoided. If you take a look at the LLVM backend where it's been put to use,
> there are all these places where people have pessimized code by moving out
> of temporaries etc.
It's hard to evaluate such a claim - it'd be best if such feedback is
provided in code review to those commits.
A cursory glance at the std::moves in llvm/lib and llvm/include most
of them seemed to be used on named variables, not temporaries. One of
only a handful of std::moves on a non-named variable I found that
seemed unnecessary (but wouldn't've pessimized code) I fixed in
r213174.
> It's not just LLVM -- check out this WebKit commit migrating away from
> std::move(), rubber-stamped by none other than our own Anders:
> http://trac.webkit.org/changeset/170774
There's certainly been some discussion about adding a warning for
redundant std::moves, which seems to be what WTF::move is going to
catch (which, weirdly, isn't what the original bug filed was
complaining about - which is an odd, but relatively harmless, issue
with std::moving const objects - unless you write some pretty weird
move ctor/move assignment, that'll do nothing). I haven't seen mention
of either of those issues coming up in the LLVM codebase as yet, but
it'd certainly be good to improve things with compiler warnings.
> So until we have a safe form of the feature I'd consider suggestions to use
> std::move() widely in the frontend, or really anywhere outside of ADT,
> actively harmful.
I think we're pretty well beyond that point already. There's ~100 uses
of std::move in Clang already, ~200 in llvm.
- David
>
>
>
>>> // Set the target CPU if specified.
>>> if (!Opts->CPU.empty() && !Target->setCPU(Opts->CPU)) {
>>>
>>> Modified: cfe/trunk/lib/Frontend/ASTUnit.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ASTUnit.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Frontend/ASTUnit.cpp (original)
>>> +++ cfe/trunk/lib/Frontend/ASTUnit.cpp Sun Jul 6 00:26:44 2014
>>> @@ -504,20 +504,17 @@ class ASTInfoCollector : public ASTReade
>>> Preprocessor &PP;
>>> ASTContext &Context;
>>> LangOptions &LangOpt;
>>> - IntrusiveRefCntPtr<TargetOptions> &TargetOpts;
>>> + std::shared_ptr<TargetOptions> &TargetOpts;
>>
>> This is curious - any idea why there's a reference to a smart pointer
>> here (rather than just a reference or pointer to the object, etc)?
>> When I saw the parameter below being taken by non-const reference to a
>> smart pointer then copied into a member I was rather confused (I'd
>> assumed the member was by value and the parameter type was just poorly
>> chosen... but now I see that the member is a reference too).
>>
>> Is the thing being pointed to changing over time? That's a bit
>> tricksy... wonder what, if any, better answer there is.
>
>
> Yes, all the utility classes in ASTUnit.cpp are following this kind of
> awkward mixin pattern. In fact the whole file is full of "interesting"
> patterns..
>
> Where were your reviews when the code was first committed? That's when it
> would have helped :-)
>
> Really, in the last few weeks I've made the first conscious attempt in years
> to fix various chunks of code like this that time forgot. It's a thankless
> job and the only way I can proceed is to be conservative -- so as much as
> I'd like to I can't just magic away known problems like these as part of a
> post-commit.
>
> It's my pleasure to explain these changes, and I'll keep doing so, but be
> conscious that in the dozen review threads since yesterday you've raised
> mostly micro-nits, and discussing them has already taken up half of my week
> and probably yours too.
>
>
>
>>> IntrusiveRefCntPtr<TargetInfo> &Target;
>>> unsigned &Counter;
>>>
>>> bool InitializedLanguage;
>>> public:
>>> - ASTInfoCollector(Preprocessor &PP, ASTContext &Context, LangOptions
>>> &LangOpt,
>>> - IntrusiveRefCntPtr<TargetOptions> &TargetOpts,
>>> - IntrusiveRefCntPtr<TargetInfo> &Target,
>>> - unsigned &Counter)
>>> - : PP(PP), Context(Context), LangOpt(LangOpt),
>>> - TargetOpts(TargetOpts), Target(Target),
>>> - Counter(Counter),
>>> - InitializedLanguage(false) {}
>>> + ASTInfoCollector(Preprocessor &PP, ASTContext &Context, LangOptions
>>> &LangOpt,
>>> + std::shared_ptr<TargetOptions> &TargetOpts,
>>> + IntrusiveRefCntPtr<TargetInfo> &Target, unsigned
>>> &Counter)
>>> + : PP(PP), Context(Context), LangOpt(LangOpt),
>>> TargetOpts(TargetOpts),
>>> + Target(Target), Counter(Counter), InitializedLanguage(false) {}
>>>
>>> bool ReadLanguageOptions(const LangOptions &LangOpts,
>>> bool Complain) override {
>>> @@ -536,10 +533,10 @@ public:
>>> // If we've already initialized the target, don't do it again.
>>> if (Target)
>>> return false;
>>> -
>>> - this->TargetOpts = new TargetOptions(TargetOpts);
>>> - Target = TargetInfo::CreateTargetInfo(PP.getDiagnostics(),
>>> - &*this->TargetOpts);
>>> +
>>> + this->TargetOpts = std::make_shared<TargetOptions>(TargetOpts);
>>> + Target =
>>> + TargetInfo::CreateTargetInfo(PP.getDiagnostics(),
>>> this->TargetOpts);
>>>
>>> updated();
>>> return false;
>>> @@ -1066,8 +1063,8 @@ bool ASTUnit::Parse(llvm::MemoryBuffer *
>>> Clang->setDiagnostics(&getDiagnostics());
>>>
>>> // Create the target instance.
>>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>>> - &Clang->getTargetOpts()));
>>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>>> if (!Clang->hasTarget()) {
>>> delete OverrideMainBuffer;
>>> return true;
>>> @@ -1565,8 +1562,8 @@ llvm::MemoryBuffer *ASTUnit::getMainBuff
>>> Clang->setDiagnostics(&getDiagnostics());
>>>
>>> // Create the target instance.
>>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>>> -
>>> &Clang->getTargetOpts()));
>>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>>> if (!Clang->hasTarget()) {
>>> llvm::sys::fs::remove(FrontendOpts.OutputFile);
>>> Preamble.clear();
>>> @@ -1832,8 +1829,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvoca
>>> Clang->setDiagnostics(&AST->getDiagnostics());
>>>
>>> // Create the target instance.
>>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>>> -
>>> &Clang->getTargetOpts()));
>>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>>> if (!Clang->hasTarget())
>>> return nullptr;
>>>
>>> @@ -2409,8 +2406,8 @@ void ASTUnit::CodeComplete(StringRef Fil
>>> ProcessWarningOptions(Diag, CCInvocation->getDiagnosticOpts());
>>>
>>> // Create the target instance.
>>> - Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>>> -
>>> &Clang->getTargetOpts()));
>>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>>> if (!Clang->hasTarget()) {
>>> Clang->setInvocation(nullptr);
>>> return;
>>>
>>> Modified: cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp (original)
>>> +++ cfe/trunk/lib/Frontend/ChainedIncludesSource.cpp Sun Jul 6 00:26:44
>>> 2014
>>> @@ -100,8 +100,8 @@ ChainedIncludesSource::create(CompilerIn
>>> std::unique_ptr<CompilerInstance> Clang(new CompilerInstance());
>>> Clang->setInvocation(CInvok.release());
>>> Clang->setDiagnostics(Diags.get());
>>> -
>>> Clang->setTarget(TargetInfo::CreateTargetInfo(Clang->getDiagnostics(),
>>> -
>>> &Clang->getTargetOpts()));
>>> + Clang->setTarget(TargetInfo::CreateTargetInfo(
>>> + Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
>>> Clang->createFileManager();
>>> Clang->createSourceManager(Clang->getFileManager());
>>> Clang->createPreprocessor(TU_Prefix);
>>>
>>> Modified: cfe/trunk/lib/Frontend/CompilerInstance.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/CompilerInstance.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Frontend/CompilerInstance.cpp (original)
>>> +++ cfe/trunk/lib/Frontend/CompilerInstance.cpp Sun Jul 6 00:26:44 2014
>>> @@ -712,7 +712,8 @@ bool CompilerInstance::ExecuteAction(Fro
>>> raw_ostream &OS = llvm::errs();
>>>
>>> // Create the target instance.
>>> - setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(),
>>> &getTargetOpts()));
>>> + setTarget(TargetInfo::CreateTargetInfo(getDiagnostics(),
>>> + getInvocation().TargetOpts));
>>> if (!hasTarget())
>>> return false;
>>>
>>>
>>> Modified: cfe/trunk/unittests/Basic/SourceManagerTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/SourceManagerTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Basic/SourceManagerTest.cpp (original)
>>> +++ cfe/trunk/unittests/Basic/SourceManagerTest.cpp Sun Jul 6 00:26:44
>>> 2014
>>> @@ -38,7 +38,7 @@ protected:
>>> SourceMgr(Diags, FileMgr),
>>> TargetOpts(new TargetOptions) {
>>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>>> }
>>>
>>> FileSystemOptions FileMgrOpts;
>>> @@ -47,7 +47,7 @@ protected:
>>> DiagnosticsEngine Diags;
>>> SourceManager SourceMgr;
>>> LangOptions LangOpts;
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> IntrusiveRefCntPtr<TargetInfo> Target;
>>> };
>>>
>>>
>>> Modified: cfe/trunk/unittests/Lex/LexerTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/LexerTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Lex/LexerTest.cpp (original)
>>> +++ cfe/trunk/unittests/Lex/LexerTest.cpp Sun Jul 6 00:26:44 2014
>>> @@ -57,7 +57,7 @@ protected:
>>> TargetOpts(new TargetOptions)
>>> {
>>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>>> }
>>>
>>> std::vector<Token> CheckLex(StringRef Source,
>>> @@ -108,7 +108,7 @@ protected:
>>> DiagnosticsEngine Diags;
>>> SourceManager SourceMgr;
>>> LangOptions LangOpts;
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> IntrusiveRefCntPtr<TargetInfo> Target;
>>> };
>>>
>>>
>>> Modified: cfe/trunk/unittests/Lex/PPCallbacksTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPCallbacksTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Lex/PPCallbacksTest.cpp (original)
>>> +++ cfe/trunk/unittests/Lex/PPCallbacksTest.cpp Sun Jul 6 00:26:44 2014
>>> @@ -116,14 +116,12 @@ public:
>>> class PPCallbacksTest : public ::testing::Test {
>>> protected:
>>> PPCallbacksTest()
>>> - : FileMgr(FileMgrOpts),
>>> - DiagID(new DiagnosticIDs()),
>>> - DiagOpts(new DiagnosticOptions()),
>>> - Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
>>> - SourceMgr(Diags, FileMgr) {
>>> - TargetOpts = new TargetOptions();
>>> + : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
>>> + DiagOpts(new DiagnosticOptions()),
>>> + Diags(DiagID, DiagOpts.get(), new IgnoringDiagConsumer()),
>>> + SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions()) {
>>
>> std::make_shared here? (removes an extra allocation and doesn't fire
>> the "raw new, treat with suspicion" hairs on the back of my (& perhaps
>> others) neck)
>
>
> Heh, there are thousands of raw new expressions throughout clang's
> constructors and not a single std::make_shared()<> so those neck hairs must
> be in overtime.
>
> The counterargument is that Field(new T) is immediately recognisable
> universal for all smart pointer initialization. In my eyes it's a wash, and
> I've decided to keep the existing initialization in this commit. I guess I
> wouldn't actively object if you'd find it rewarding to tweak this line
> though.
>
> Alp.
>
>
>
>
>>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>>> }
>>>
>>> FileSystemOptions FileMgrOpts;
>>> @@ -133,7 +131,7 @@ protected:
>>> DiagnosticsEngine Diags;
>>> SourceManager SourceMgr;
>>> LangOptions LangOpts;
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> IntrusiveRefCntPtr<TargetInfo> Target;
>>>
>>> // Register a header path as a known file and add its location
>>>
>>> Modified: cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp?rev=212388&r1=212387&r2=212388&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp
>>> (original)
>>> +++ cfe/trunk/unittests/Lex/PPConditionalDirectiveRecordTest.cpp Sun Jul
>>> 6 00:26:44 2014
>>> @@ -38,7 +38,7 @@ protected:
>>> TargetOpts(new TargetOptions)
>>> {
>>> TargetOpts->Triple = "x86_64-apple-darwin11.1.0";
>>> - Target = TargetInfo::CreateTargetInfo(Diags, &*TargetOpts);
>>> + Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
>>> }
>>>
>>> FileSystemOptions FileMgrOpts;
>>> @@ -47,7 +47,7 @@ protected:
>>> DiagnosticsEngine Diags;
>>> SourceManager SourceMgr;
>>> LangOptions LangOpts;
>>> - IntrusiveRefCntPtr<TargetOptions> TargetOpts;
>>> + std::shared_ptr<TargetOptions> TargetOpts;
>>> IntrusiveRefCntPtr<TargetInfo> Target;
>>> };
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>
More information about the cfe-commits
mailing list