awarzynski added a comment. In D123211#3435421 <https://reviews.llvm.org/D123211#3435421>, @rovka wrote:
> Is there actually a significant difference, besides the naming (which is easy > to change)? AFAICT the BackendAction isn't initializing anything > backend-related except very close to where it's using it, so that can happen > inside a switch/if branch. Currently the driver does the bare minimum to generate machine code, hence in practice both actions are near identical. I expect this to change, but I might be wrong :) > I was thinking also about the code for generating the output file, which can > be folded into the switch from BackendAction. If you consider that too, it > becomes a very large percentage of EmitLLVMBitcodeAction::ExecuteAction that > can be shared. Ack! > Ok. IMO the template method pattern would work well here (or less formally, > just a simple switch to the same effect), but I can understand if you think > it's premature to go that route. Definitely not objecting code re-use! :) But I would like to wait for a few patches for optimisation pipelines to land first. And if anyone feels differently and sends a patch, I'd be alright with that. ================ Comment at: flang/lib/Frontend/FrontendActions.cpp:491 + // Create and configure `TargetMachine` + std::unique_ptr<llvm::TargetMachine> TM; + ---------------- 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? 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
