tejohnson added a comment.
@MaskRay wondering if this is a good change to make for ELF as well, wdyt?
================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1104
- auto AddStream = [&](size_t Task) {
+ auto AddStream = [&](size_t Task, Twine File) {
return std::make_unique<CachedFileStream>(std::move(OS),
----------------
zequanwu wrote:
> tejohnson wrote:
> > I think you might need to mark the new parameter as unused here and in
> > other cases where it isn't used via LLVM_ATTRIBUTE_UNUSED, to avoid build
> > warnings - I can't recall how strictly that is enforced.
> LLVM_ATTRIBUTE_UNUSED is used to suppress unused functions. For unused
> parameter, I don't see any build warnings when compiling with this patch. I
> feel like it's very common to have unused parameter.
LLVM_ATTRIBUTE_UNUSED maps to __attribute__((__unused__)) which works for
either functions or parameters. However, I see where the macro is defined in
llvm/include/llvm/Support/Compiler.h that using "(void)unused;" is preferred.
Either one works (see below). However, it must be the case that there are no
bots building with -Wunused-parameter, since I see Task is also unused here. So
nevermind this suggestion, since what you have currently is consistent with
what is already done here.
```
$ cat unused.cc
int foo(int x1, int x2 __attribute__((__unused__)), int x3) {
(void)x3;
return 1;
}
$ clang++ -c unused.cc -Wunused-parameter
unused.cc:1:13: warning: unused parameter 'x1' [-Wunused-parameter]
int foo(int x1, int x2 __attribute__((__unused__)), int x3) {
^
1 warning generated.
```
================
Comment at: lld/COFF/LTO.cpp:229
+ StringRef ltoObjName;
+ if (bitcodeFilePath == "ld-temp.o") {
+ ltoObjName =
----------------
zequanwu wrote:
> tejohnson wrote:
> > This case should always be i==0 I think?
> IIUC, "ld-temp.o" is the name of combined module. Do you mean there will be
> only 1 combined module and it will always be the first task?
Yes. So you don't need the handling for i==0 in the name in this case (you
could probably assert that i==0).
================
Comment at: llvm/include/llvm/Support/Caching.h:53
///
/// if (AddStreamFn AddStream = Cache(Task, Key))
/// ProduceContent(AddStream);
----------------
tejohnson wrote:
> This and possibly other header file descriptions need updates.
Can you add a note that ModuleName is the unique module identifier for the
bitcode module the cache is being checked for.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137217/new/
https://reviews.llvm.org/D137217
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
