ekieri added inline comments.
================ Comment at: flang/lib/Frontend/FrontendActions.cpp:491 + // Create and configure `TargetMachine` + std::unique_ptr<llvm::TargetMachine> TM; + ---------------- awarzynski wrote: > ekieri wrote: > > awarzynski wrote: > > > ekieri wrote: > > > > Is there a reason why use TM.reset instead of initialising TM like you > > > > do with os below? > > > Good question! > > > > > > Note that [[ > > > https://github.com/llvm/llvm-project/blob/3d0e0e1027203fe5e89104ad81ee7bb53e525f95/llvm/include/llvm/MC/TargetRegistry.h#L446-L452 > > > | createTargetMachine ]] that I use below returns a plain pointer. [[ > > > https://github.com/llvm/llvm-project/blob/3d0e0e1027203fe5e89104ad81ee7bb53e525f95/clang/include/clang/Frontend/CompilerInstance.h#L703-L706 > > > | createDefaultOutputFile ]] that I use for initialising `os` below > > > returns `std::unique_ptr<raw_pwrite_stream>`. IIUC, I can only [[ > > > https://en.cppreference.com/w/cpp/memory/unique_ptr/reset | reset ]] a > > > `unique_ptr` (like `TM`) with a plain pointer. > > > > > > Have I missed anything? > > Well, I had missed that. Thanks! Let's see if I got it right this time. > > > > So unique_ptr has an _explicit_ constructor taking a raw pointer > > ([[https://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr | > > constructor (2)]]). So as you say (if I interpreted you correctly), > > > > std::unique_ptr<llvm::TargetMachine> TM = > > theTarget->createTargetMachine(...); > > > > does not work, as it requires an implicit conversion from raw to unique > > pointer. But constructor syntax should (and does for me) work: > > > > std::unique_ptr<llvm::TargetMachine> > > TM(theTarget->createTargetMachine(...)); > > > > as this makes the conversion explicit. Does this make sense, or did I miss > > something more? > > Does this make sense, or did I miss something more? > > Yes, I'm the one who missed something here :) Many thanks for pointing this > out! > > I convinced myself that I checked this, but clearly I didn't. And I confused > the copy-assignment with a proper constructor. Every day is a school day! > > I'll merge this shortly and I will incorporate your suggestion. Thanks for > your patience going over this with me :) > > Well, thank you! I also learned something new. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123211/new/ https://reviews.llvm.org/D123211 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
