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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, reverted and looking into it.


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

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

The test fails on mac and win:
http://45.33.8.238/mac/16341/step_9.txt
http://45.33.8.238/win/18704/step_9.txt


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

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

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rG9963d93b0731: [clangd] Config: config struct propagated 
through Context (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D82606?vs=273543=274194#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82606

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/Config.cpp
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -7,7 +7,9 @@
 //===--===//
 
 #include "CompileCommands.h"
+#include "Config.h"
 #include "TestFS.h"
+#include "support/Context.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
@@ -185,6 +187,26 @@
 }
 #endif
 
+TEST(CommandMangler, ConfigEdits) {
+  auto Mangler = CommandMangler::forTests();
+  std::vector Cmd = {"clang++", "foo.cc"};
+  {
+Config Cfg;
+Cfg.CompileFlags.Edits.push_back([](std::vector ) {
+  for (auto  : Argv)
+for (char  : Arg)
+  C = llvm::toUpper(C);
+});
+Cfg.CompileFlags.Edits.push_back(
+[](std::vector ) { Argv.push_back("--hello"); });
+WithContextValue WithConfig(Config::Key, std::move(Cfg));
+Mangler.adjust(Cmd);
+  }
+  // Edits are applied in given order and before other mangling.
+  EXPECT_THAT(Cmd,
+  ElementsAre("CLANG++", "FOO.CC", "--hello", "-fsyntax-only"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Config.h
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.h
@@ -0,0 +1,64 @@
+//===--- Config.h - User configuration of clangd behavior *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// This file defines "resolved" configuration seen by features within clangd.
+// For example, settings may vary per-file, the resolved Config only contains
+// settings that apply to the current file.
+//
+// This is distinct from how the config is specified by the user (Fragment)
+// interpreted (CompiledFragment), and combined (Provider).
+// ConfigFragment.h describes the steps to add a new configuration option.
+//
+// Because this structure is shared throughout clangd, it's a potential source
+// of layering problems. Config should be expressed in terms of simple
+// vocubulary types where possible.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+
+#include "support/Context.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Settings that express user/project preferences and control clangd behavior.
+///
+/// Generally, features should consume config() and the caller is responsible
+/// for setting it appropriately. In practice these callers are ClangdServer,
+/// TUScheduler, and BackgroundQueue.
+struct Config {
+  /// Context key which can be used to set the current Config.
+  static clangd::Key Key;
+
+  Config() = default;
+  Config(const Config &) = delete;
+  Config =(const Config &) = delete;
+  Config(Config &&) = default;
+  Config =(Config &&) = default;
+
+  /// Controls how the compile command for the current file is determined.
+  struct {
+// Edits to apply to the compile command, in sequence.
+// FIXME: these functions need to be const-callable. For now, const_cast.
+std::vector &)>> Edits;
+  } CompileFlags;
+};
+
+/// Returns the Config of the current Context, or an empty configuration.
+const Config ();
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/Config.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.cpp
@@ -0,0 +1,25 @@
+//===--- Config.cpp - User configuration of clangd behavior ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH 

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

[Sam McCall via Phabricator via cfe-commits]

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  : const_cast(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

[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  : const_cast(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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment 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  : const_cast(Config::current()).CompileFlags.Edits)
+Edit(Cmd);

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. 


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

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks, LGTM. Just a question around the order of config vs other mangling.




Comment at: clang-tools-extra/clangd/CompileCommands.cpp:187
+  // FIXME: remove const_cast once unique_function is const-compatible.
+  for (auto  : const_cast(Config::current()).CompileFlags.Edits)
+Edit(Cmd);

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.


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

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

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 273543.
sammccall added a comment.

config() -> Config::current(). "config" already names a namespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82606

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/Config.cpp
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -7,7 +7,9 @@
 //===--===//
 
 #include "CompileCommands.h"
+#include "Config.h"
 #include "TestFS.h"
+#include "support/Context.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
@@ -185,6 +187,26 @@
 }
 #endif
 
+TEST(CommandMangler, ConfigEdits) {
+  auto Mangler = CommandMangler::forTests();
+  std::vector Cmd = {"clang++", "foo.cc"};
+  {
+Config Cfg;
+Cfg.CompileFlags.Edits.push_back([](std::vector ) {
+  for (auto  : Argv)
+for (char  : Arg)
+  C = llvm::toUpper(C);
+});
+Cfg.CompileFlags.Edits.push_back(
+[](std::vector ) { Argv.push_back("--hello"); });
+WithContextValue WithConfig(Config::Key, std::move(Cfg));
+Mangler.adjust(Cmd);
+  }
+  // Edits are applied in given order and before other mangling.
+  EXPECT_THAT(Cmd,
+  ElementsAre("CLANG++", "FOO.CC", "--hello", "-fsyntax-only"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Config.h
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.h
@@ -0,0 +1,63 @@
+//===--- Config.h - User configuration of clangd behavior *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// This file defines "resolved" configuration seen by features within clangd.
+// For example, settings may vary per-file, the resolved Config only contains
+// settings that apply to the current file.
+//
+// This is distinct from how the config is specified by the user (Fragment)
+// interpreted (CompiledFragment), and combined (Provider).
+// ConfigFragment.h describes the steps to add a new configuration option.
+//
+// Because this structure is shared throughout clangd, it's a potential source
+// of layering problems. Config should be expressed in terms of simple
+// vocubulary types where possible.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+
+#include "support/Context.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Settings that express user/project preferences and control clangd behavior.
+///
+/// Generally, features should consume Config::current() and the caller is
+/// responsible for setting it appropriately. In practice these callers are
+/// ClangdServer, TUScheduler, and BackgroundQueue.
+struct Config {
+  /// Returns the Config of the current Context, or an empty configuration.
+  static const Config ();
+  /// Context key which can be used to set the current Config.
+  static clangd::Key Key;
+
+  Config() = default;
+  Config(const Config &) = delete;
+  Config =(const Config &) = delete;
+  Config(Config &&) = default;
+  Config =(Config &&) = default;
+
+  /// Controls how the compile command for the current file is determined.
+  struct {
+// Edits to apply to the compile command, in sequence.
+// FIXME: these functions need to be const-callable. For now, const_cast.
+std::vector &)>> Edits;
+  } CompileFlags;
+};
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/Config.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.cpp
@@ -0,0 +1,25 @@
+//===--- Config.cpp - User configuration of clangd behavior ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Config.h"
+#include "support/Context.h"
+

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

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: hokein, kadircet.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov, mgorny.
Herald added a project: clang.

This introduces the "semantic form" of config exposed to features,
contrasted with the "syntactic form" exposed to users in e9fb1506b83d 
.

The two are not connected, CompiledFragment and Provider will bridge that gap.
Nor is configuration actually set: that needs changes to ClangdServer,
TUScheduler, and BackgroundQueue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82606

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/CompileCommands.cpp
  clang-tools-extra/clangd/Config.cpp
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp

Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -7,7 +7,9 @@
 //===--===//
 
 #include "CompileCommands.h"
+#include "Config.h"
 #include "TestFS.h"
+#include "support/Context.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringExtras.h"
@@ -185,6 +187,26 @@
 }
 #endif
 
+TEST(CommandMangler, ConfigEdits) {
+  auto Mangler = CommandMangler::forTests();
+  std::vector Cmd = {"clang++", "foo.cc"};
+  {
+Config Cfg;
+Cfg.CompileFlags.Edits.push_back([](std::vector ) {
+  for (auto  : Argv)
+for (char  : Arg)
+  C = llvm::toUpper(C);
+});
+Cfg.CompileFlags.Edits.push_back(
+[](std::vector ) { Argv.push_back("--hello"); });
+WithContextValue WithConfig(Config::Key, std::move(Cfg));
+Mangler.adjust(Cmd);
+  }
+  // Edits are applied in given order and before other mangling.
+  EXPECT_THAT(Cmd,
+  ElementsAre("CLANG++", "FOO.CC", "--hello", "-fsyntax-only"));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/Config.h
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.h
@@ -0,0 +1,64 @@
+//===--- Config.h - User configuration of clangd behavior *- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Various clangd features have configurable behaviour (or can be disabled).
+// This file defines "resolved" configuration seen by features within clangd.
+// For example, settings may vary per-file, the resolved Config only contains
+// settings that apply to the current file.
+//
+// This is distinct from how the config is specified by the user (Fragment)
+// interpreted (CompiledFragment), and combined (Provider).
+// ConfigFragment.h describes the steps to add a new configuration option.
+//
+// Because this structure is shared throughout clangd, it's a potential source
+// of layering problems. Config should be expressed in terms of simple
+// vocubulary types where possible.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CONFIG_H
+
+#include "support/Context.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// Settings that express user/project preferences and control clangd behavior.
+///
+/// Generally, features should consume config() and the caller is responsible
+/// for setting it appropriately. In practice these callers are ClangdServer,
+/// TUScheduler, and BackgroundQueue.
+struct Config {
+  /// Context key which can be used to set the current Config.
+  static clangd::Key Key;
+
+  Config() = default;
+  Config(const Config &) = delete;
+  Config =(const Config &) = delete;
+  Config(Config &&) = default;
+  Config =(Config &&) = default;
+
+  /// Controls how the compile command for the current file is determined.
+  struct {
+// Edits to apply to the compile command, in sequence.
+// FIXME: these functions need to be const-callable. For now, const_cast.
+std::vector &)>> Edits;
+  } CompileFlags;
+};
+
+/// Returns the Config of the current Context, or an empty configuration.
+const Config ();
+
+} // namespace clangd
+} // namespace clang
+
+#endif
Index: clang-tools-extra/clangd/Config.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/Config.cpp
@@ -0,0 +1,25 @@
+//===--- Config.cpp - User configuration of clangd