[PATCH] D123211: [flang][driver] Add support for generating LLVM bytecode files

[Emil Kieri via Phabricator via cfe-commits]

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