[PATCH] D92012: [clangd][query-driver] Extract target

[Kadir Cetinkaya via Phabricator via cfe-commits]

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