kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:67
+
+bool isValidTarget(llvm::StringRef Triple) {
+ std::shared_ptr<TargetOptions> TargetOpts(new TargetOptions);
----------------
i think you can just do `TargetRegistry::lookupTarget`
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:230
+ Driver, llvm::join(Info->SystemIncludes, ", "));
+ if (!Info->Target.empty())
+ log("Target extractor: successfully executed {0}, got target: "
----------------
nit: I would fold these two into a single log statement.
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:337
- std::vector<std::string> SystemIncludes =
+ llvm::Optional<DriverInfo> Info =
QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
----------------
nit:
```
if(auto Info = ...)
setTarget(addSystemIncludes(...), ...);
return std::move(Cmd);
```
================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212
+ if (!Target.empty()) {
+ Cmd.CommandLine.push_back("-target");
+ Cmd.CommandLine.push_back(Target);
----------------
ArcsinX wrote:
> ArcsinX wrote:
> > kadircet wrote:
> > > sammccall wrote:
> > > > `clang -target foo test.cc` seems to be a hard error in the driver if
> > > > the target is unknown.
> > > > (vs likely *some* functionality if we just didn't set the driver)
> > > >
> > > > so this could regress some scenarios. Can we mitigate that?
> > > > (It's possible that we're running the driver in a mode where we proceed
> > > > anyway, but I can't remember :-()
> > > what if target already exists in `Cmd`?
> > >
> > > also it would be nice to use `--target=X` format to be consistent with
> > > target inference from invocation name as in
> > > https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Tooling.cpp#L278.
> > I failed to find options to process in case of invalid target, so added
> > target validation.
> We could specify several --target options, the last one will be used. But I
> am not sure should we override existing or not.
> We could specify several --target options, the last one will be used. But I
> am not sure should we override existing or not.
Having multiple targets sounds confusing. I would prefer keeping a target
specifically mentioned by the user but it is also possible for the target to be
inferred by clang (and that might be wrong). This is similar to our stand
against predefined macros though, i think we should fix clang's inference logic
if it has any false positives. So i am slightly leaning towards not overriding
the target if it exists.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92012/new/
https://reviews.llvm.org/D92012
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
