[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

[Nathan Ridge via Phabricator via llvm-commits]

nridge added a comment.

In D138546#4053538 <https://reviews.llvm.org/D138546#4053538>, @cpsauer wrote:

> @nridge, I took a shot at the change you suggested. Confirming that this what you were thinking of, removing `inferTargetAndDriverMode` from database load and replacing it with a call to the underlying `addTargetAndModeForProgramName` in the `CommandMangler` after the `SystemIncludeExtractor` is invoked?

Yup, that's what I had in mind. Thanks for writing that up!

> Note that I also saw a parallel call to `inferTargetAndDriverMode` in `JSONCompilationDatabasePlugin`, which seemed like it might cause the same problem, so I eliminated the call in there, too.

Oh, huh, I didn't realize that `JSONCompilationDatabasePlugin` has this behaviour baked in.

I would err on the side of leaving that call site as is.

My thinking is as follows:

- The general purpose of the compilation database utilities in libTooling is to produce command lines that will be fed to the clang driver library.
- The clang driver does support `--target`, and in some cases having that flag present is important.
- For clangd's call site, we're not //removing// the logic to add the `--target` flag, we're just moving it from an ealier stage of the pipeline to a later stage of the pipeline, because our pipeline contains a specialized step (SystemIncludeExtractor) that may involve executing a gcc driver with (parts of the) the command.
- But for other tools built on libTooling that use `JSONCompilationDatabasePlugin`, this change would be removing the `--target` logic altogether, which could regress behaviour.

> I then culled the dead code, since there weren't other call sites for the `addTargetAndModeForProgramName` wrappings.

Reagrdless of what we decide about `JSONCompilationDatabasePlugin`, we probably shouldn't remove `clang::tooling::inferTargetAndDriverMode()`, as that's a public libTooling API that may be used by external projects that use libTooling as a library.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138546/new/

https://reviews.llvm.org/D138546