abhina.sreeskantharajan added inline comments.
================
Comment at: clang/include/clang/Frontend/CompilerInstance.h:738-740
+ createOutputFileImpl(StringRef OutputPath, llvm::sys::fs::OpenFlags Flags,
bool RemoveFileOnSignal, bool UseTemporary,
bool CreateMissingDirectories);
----------------
dexonsmith wrote:
> abhina.sreeskantharajan wrote:
> > rnk wrote:
> > > I think this is only going to be worth it if we can roll up all of these
> > > booleans into a new flags enum for compiler instance. It also prevents
> > > introducing a new use of FileSystem.h, which is an expensive header to
> > > include.
> > Sorry, I don't think I completely understand your suggestion. Are you
> > proposing that we create a new enum just for CompilerInstance.h for the
> > other booleans like RemoveFileOnSignal, UseTemporary,
> > CreateMissingDirectories? And this new enum also contain text/binary flags
> > so we don't need to use the enum in FileSystem.h?
> >
> > For background, I need this specific change to set some more text files
> > with OF_Text because my old commit got reverted since it caused CRLF issues
> > on Windows https://reviews.llvm.org/D96363.
> >
> Yes, I think that's what @rnk is suggesting, maybe along the lines of the
> OutputConfigFlag in the (languishing, I need to get back to it)
> OutputBackend/OutputManager proposal I have (see, e.g.,
> https://reviews.llvm.org/D95501, which needs to be split up), or maybe as a
> struct with named members as @sammccall suggests there.
>
> > It also prevents introducing a new use of FileSystem.h, which is an
> > expensive header to include.
> Another option that'd be more isolated would be to split the OpenFlag enum
> (and/or its friends) out to another header in
> llvm/include/llvm/Support/FileSystem/ (say,
> .../FileSystem/FileSystemEnums.h), like was done for UniqueID.h.
Thanks for clarifying. I think having a duplicate copy of the OpenFlags may be
confusing. I like the idea of putting the enums in its own header file. But can
that be done in a separate change than this patch? It will be a much larger
change touching more files than this patcch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100872/new/
https://reviews.llvm.org/D100872
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
