sammccall marked 2 inline comments as done. sammccall added inline comments.
================ 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); ---------------- kadircet wrote: > 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) Yeah logging earlier would be nice but there's a layering problem - CDB doesn't know if this is a file we're actually editing, and we only want to log if it is. I think this is annoying, but not urgent to solve. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82606/new/ https://reviews.llvm.org/D82606 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
