stevewan marked 4 inline comments as done.
stevewan added inline comments.
================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+ const char *LinkingOutput) const {
+ claimNoWarnArgs(Args);
+ ArgStringList CmdArgs;
----------------
Xiangling_L wrote:
> The definition of `claimNoWarnArgs` is to suppress warnings for some options
> if they are unused, can you explain a little bit about how did you figure out
> that we don't want warnings for those?
>
> Some context of `claimNoWarnArgs`:
>
> ```
> // Claim options we don't want to warn if they are unused. We do this for
> // options that build systems might add but are unused when assembling or only
> // running the preprocessor for example.
> void tools::claimNoWarnArgs(const ArgList &Args) {
> // Don't warn about unused -f(no-)?lto. This can happen when we're
> // preprocessing, precompiling or assembling.
> Args.ClaimAllArgs(options::OPT_flto_EQ);
> Args.ClaimAllArgs(options::OPT_flto);
> Args.ClaimAllArgs(options::OPT_fno_lto);
> }
> ```
>
The original reason was that, since we are doing assembling here, and as stated
in `claimNoWarnArgs`, there might be an unused `-f(no-)?lto` when we're
assembling, and we'd like to suppress the warning(s) for that.
Looking into it, the three options [[
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-flto
| `flto`, `fno_lto` ]] , and [[
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-flto
| `flto_EQ` ]] are used to enable/disable link time optimization, but LTO is
not supported by the AIX system linker. Driver would just throw error if the
user passes it `-flto` or `-flto=<arg>` on AIX, so claiming them or not
currently makes no actual difference as far as I'm concerned.
That being said, I don't have a strong opinion either way. Let's see how other
people think.
================
Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+ bool hasIntegratedCPP() const override { return false; }
+
----------------
daltenty wrote:
> stevewan wrote:
> > Xiangling_L wrote:
> > > I saw a lot of other target also set `hasIntegratedCPP()` as false, but I
> > > didn't find any explanation in documentation, so I am curious that what
> > > this is about?
> > I also failed to find useful resources that explains the purpose of this
> > function. The main rationales of adding it were essentially that,
> > 1. It's a pure virtual function in the base `Tool` class,
> > 2. Most if not all of other targets had overridden this function such that
> > it returns false.
> >
> > I'll leave this comment open, and see if someone could enlighten us.
> Only "Compiler" tools set hasIntegratedCPP() to true. Looking into it, it
> seems combineWithPreprocessor() uses this to decide whether the tool supports
> preprocessor actions so it may fold them in to one step. I think we are safe
> leaving this as is.
I checked this function on other targets. As David pointed out, only the
compiler tools would set hasIntegratedCPP() to true, the assembler and linker
tools set it to false because they do not support combining with preprocessor
action.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69620/new/
https://reviews.llvm.org/D69620
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
