steven_wu added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:164
LLVMIRGenerationRefCount(0),
- Gen(CreateLLVMCodeGen(Diags, InFile, std::move(FS), HeaderSearchOpts,
- PPOpts, CodeGenOpts, C, CoverageInfo)),
+ Gen(CreateLLVMCodeGen(Diags, InFile, FS, HeaderSearchOpts, PPOpts,
+ CodeGenOpts, C, CoverageInfo)),
----------------
benlangmuir wrote:
> Wouldn't `move` be fine here since it's already copied to `this->FS`?
I think I confused myself for which `FS` is moved here. I renamed the parameter
so it is clear move is applied to the FileSystem passed in.
================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1729
+ if (!Opts.ProfileInstrumentUsePath.empty()) {
+ auto FS = llvm::vfs::getRealFileSystem();
+ setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, *FS, Diags);
----------------
akyrtzi wrote:
> Could we propagate the VFS that the `CompilerInvocation` is going to create
> here? Otherwise this seems like a hidden "landmine" that someone is bound to
> trip on later on.
Currently, Profile look up doesn't go through any VFS, so vfs overlay has no
effect on profiles. Putting through VFS is changing behavior, even I think it
is for good.
It also has the potential to make code harder to read because creating VFS
relies on a compiler invocation which we are creating here.
Let me add a fixme here first.
================
Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:20
#include "llvm/Support/Discriminator.h"
+#include "llvm/Support/VirtualFileSystem.h"
#include <memory>
----------------
akyrtzi wrote:
> Recommend to forward declare instead of including the header.
The default parameter needs to instantiate here and pretty much all references
to `PGOOptions` has `Optional<PGOOptions>` which requires including VFS. I will
leave this one since `PGOOptions.h` is a rare header to include.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139052/new/
https://reviews.llvm.org/D139052
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
