[PATCH] D82606: [clangd] Config: config struct propagated through Context

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

oops, thought I've stamped it last time.



================
Comment at: clang-tools-extra/clangd/CompileCommands.cpp:187
+  // FIXME: remove const_cast once unique_function is const-compatible.
+  for (auto &Edit : const_cast<Config &>(Config::current()).CompileFlags.Edits)
+    Edit(Cmd);
----------------
sammccall wrote:
> kadircet wrote:
> > what's the rationale behind applying this before any other mangling?
> > 
> > I can see that the rest of the mangling happens to make sure clangd works out-of-the-box for "more" users, so should be safe to apply as a final step.
> > But on the other hand, applying config after those would give the user full control over the final command, which I believe is equally important.
> I'll be honest, I don't really know which is better here. The differences are subtle, and there are arguments for each. I think we should probably just pick one and be open to changing it later.
> 
> My reasoning for this behavior: currently the user view of compile commands is basically "strings in compile_commands.json", and this mangling we do is best thought of as modifying the behavior of the driver. E.g. in an ideal world `-fsyntax-only` would not be a flag, we'd just use APIs that imply that behavior.
> In this view of the world, the user is expected to understand compile commands + tweaks but not the mangling, so placing tweaks after mangling means they can't really reason about the transformations. And it allows stripping structurally important things we inject like `fsyntax-only` which seems wrong.
> 
> This argument works better for some args/manglings than others, and the way we log args cuts against it a bit too. 
SG, as you mentioned in the last paragraph I would be looking at logs to figure out what my compile commands for a file are, but may be it's just me. Hence having this tweaking in the middle was a little bit surprising. (Moreover, if one day we decide to have build system integrations it might imply there won't be any written compile_commands.json, but we'll rather fetch them on the fly and logs might be the only way to look at those commands. Even in such a scenario, I suppose changing the way we log might be a better approach because we indeed do more manipulations even after logging e.g. turning off preamble related options)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82606