[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rGf12cd99c440a: [clangd] Config: compile Fragment ->
CompiledFragment -> Config (authored by sammccall).
Changed prior to commit:
https://reviews.llvm.org/D82612?vs=274623&id=274713#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Rng.emplace();
-Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-Out.Rng->start.character = R.first;
-Out.Rng->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Rng;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
@@ -101,8 +51,8 @@
auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_EQ(Results.size(), 2u);
- EXPECT_FALSE(Results.front().Condition.HasUnrecognizedCondition);
- EXPECT_THAT(Results.front().Condition.PathMatch, ElementsAre(Val("abc")));
+ EXPECT_FALSE(Results.front().If.HasUnrecognizedCondition);
+ EXPECT_THAT(Results.front().If.PathMatch, ElementsAre(Val("abc")));
EXPECT_THAT(Results.front().CompileFlags.Add,
ElementsAre(Val("foo"), Val("bar")));
@@ -120,7 +70,7 @@
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_EQ(Results.size(), 1u);
ASSERT_NE(Results.front().Source.Manager, nullptr);
- EXPECT_EQ(toRange(Results.front().Condition.PathMatch.front().Range,
+ EXPECT_EQ(toRange(Results.front().If.PathMatch.front().Range,
*Results.front().Source.Manager),
YAML.range());
}
@@ -140,7 +90,7 @@
ASSERT_THAT(
Diags.Diagnostics,
- ElementsAre(AllOf(DiagMessage("Unknown Condition key UnknownCondition"),
+ ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"),
DiagKind(llvm::SourceMgr::DK_Warning),
DiagPos(YAML.range().start), DiagRange(YAML.range())),
AllOf(DiagMessage("Unexpected token. Expected Key, Flow "
@@ -150,7 +100,7 @@
ASSERT_EQ(Results.size(), 2u);
EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
- EXPECT_TRUE(Results.front().Condition.HasUnrecognizedCondition);
+ EXPECT_TRUE(Results.front().If.HasUnrecognizedCondition);
EXPECT_THAT(Results.back().CompileFlags.Add, IsEmpty());
}
Index: clang-tools-ex
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall marked 8 inline comments as done.
sammccall added a comment.
Thanks a lot for the review, I think this is a lot better now.
And particularly on where docs are lacking - I'm happy to write comments but
it's hard for me to know which bits are least clear.
Comment at: clang-tools-extra/clangd/ConfigFragment.h:100
/// Each separate condition must match (combined with AND).
/// When one condition has multiple values, any may match (combined with OR).
///
hokein wrote:
> I think this comment is important, but it is a bit vague (not quite friendly
> to readers without much config context), we might want to refine it -- would
> be nice to clearly state what's a condition, e.g. "PathMatch" is a condition,
> "Platform" is a condition.
>
>
> maybe separate the rename to a separate patch, but up to you.
Updated comment to tie the concept of conditions to the contents of the If
block, and added an example for the OR matching.
Comment at: clang-tools-extra/clangd/ConfigFragment.h:108
+ struct IfBlock {
/// The file being processed must fully match a regular expression.
std::vector> PathMatch;
hokein wrote:
> nit: maybe explicitly spell this is an `OR` match.
I'd prefer not to do this explicitly in each condition, as it's hard to get
across in a couple of words and I think it'd hurt readability overall to repeat
it so much.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.
Thanks, looks good.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > I think if this case happened, we might just bail out directly --
> > > > adding the regex conditions seems unnecessary, we never execute them on
> > > > Line 127 (but we might still need computing the regex for diagnostics).
> > > >
> > > > and I'm not sure the motivation of using `vector` for `Conditions` and
> > > > `Apply`, it looks like Apply is just 1 element (if we do the bailout
> > > > for conditions, `Conditions` will be at most 1 element).
> > > >
> > > > I feel like using a single unique_function for `Conditions` and `Apply`
> > > > might be a better fit with `Fragment`. There is a comment in
> > > > `ConfigFragment.h`:
> > > >
> > > > > Each separate condition must match (combined with AND).
> > > > > When one condition has multiple values, any may match (combined with
> > > > > OR).
> > > >
> > > > so my reading is that:
> > > >
> > > > - `Fragment` and `CompiledFragment` is a 1:1 mapping
> > > > - A `Fragment::Condition` represents a single condition, so the AND
> > > > match applies for different `Fragment`s, which is in ConfigProvider?
> > > > - A single condition can have multiple values (`PathMatch`), we use OR
> > > > match
> > > >
> > > > Is that correct?
> > > > I think if this case happened, we might just bail out directly --
> > > > adding the regex conditions seems unnecessary, we never execute them on
> > > > Line 127 (but we might still need computing the regex for diagnostics).
> > > >
> > > > and I'm not sure the motivation of using vector for Conditions and
> > > > Apply, it looks like Apply is just 1 element (if we do the bailout for
> > > > conditions, Conditions will be at most 1 element).
> > >
> > > So the explanation for both of these things is that this file is going to
> > > grow a lot, and in particular these sections handling PathMatch etc are
> > > going to occur again and again, so we need a scalable pattern. I've
> > > deliberately limited the scope of the first patchset to only support one
> > > condition, one setting to apply etc, but you have to imagine there are
> > > likely to be 5 conditions and maybe 25 settings.
> > >
> > > So with that in mind:
> > > - yes, actually saving the conditions isn't necessary, but we do need to
> > > evaluate them for side-effects (diagnostics) and it's simpler not to keep
> > > track of them. Optimizing performance of broken configs is not a big win
> > > :-)
> > > - it's marginally simpler to have only one Apply function, but much
> > > simpler if separate parts of compile() can push settings independently.
> > > Same goes for condition in fact...
> > >
> > > > Fragment and CompiledFragment is a 1:1 mapping
> > >
> > > Yes. I think this is somewhere where the naming is helpful at least :-)
> > >
> > > > A Fragment::Condition represents a single condition, so the AND match
> > > > applies for different Fragments, which is in ConfigProvider?
> > >
> > > The AND match applies to the multiple Conditions in a single
> > > CompiledFragment. Each fragment only considers its own conditions.
> > > So given:
> > > ```
> > > If:
> > > Platform: win
> > > PathMatch: [foo1/.*, foo2/.*]
> > > CompileFlags:
> > > Add: -foo
> > > ---
> > > CompileFlags:
> > > Add: -bar
> > > ```
> > >
> > > We end up with... one Provider, suppling two CompiledFragments (each
> > > compiled from a separate Fragment).
> > >
> > > The first CompiledFragment has two Conditions, conceptually:
> > > - `Params.Platform == "win"`
> > > - `Regex("foo1/.*").matches(Params.File) ||
> > > Regex("foo2/.*").matches(Params.File)`
> > >
> > > The second CompiledFragment has no Conditions. Each fragment has one
> > > Apply function.
> > >
> > > The CompiledFragments are applied independently. They each consider their
> > > own Conditions, ANDing them together.
> > >
> > > So CompiledFragment #1 checks both conditions. If any fails, it bails
> > > out. Otherwise it runs its Apply function, adding `-foo`.
> > >
> > > Then CompiledFragment #2 runs. It has no conditions, so never bails out.
> > > It adds `-bar`.
> > oh, thanks for the explanation, this helps me a lot to understand the code.
> >
> > so the member `Condition` in `Fragment` will be enhanced to something like
> > `std::vector Conditions` in the future? If so, can we defer that for
> > `CompiledFragment` as well?
> >
> >
> > Since `Fragment` and `CompiledFragment` is 1:1 mapping, I think it is
> > important to keep their implementation aligned, it'd help a lot for readers
> > to understand the code. The condition logic (AND, OR) is subtle and
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > I think if this case happened, we might just bail out directly -- adding
> > > the regex conditions seems unnecessary, we never execute them on Line 127
> > > (but we might still need computing the regex for diagnostics).
> > >
> > > and I'm not sure the motivation of using `vector` for `Conditions` and
> > > `Apply`, it looks like Apply is just 1 element (if we do the bailout for
> > > conditions, `Conditions` will be at most 1 element).
> > >
> > > I feel like using a single unique_function for `Conditions` and `Apply`
> > > might be a better fit with `Fragment`. There is a comment in
> > > `ConfigFragment.h`:
> > >
> > > > Each separate condition must match (combined with AND).
> > > > When one condition has multiple values, any may match (combined with
> > > > OR).
> > >
> > > so my reading is that:
> > >
> > > - `Fragment` and `CompiledFragment` is a 1:1 mapping
> > > - A `Fragment::Condition` represents a single condition, so the AND match
> > > applies for different `Fragment`s, which is in ConfigProvider?
> > > - A single condition can have multiple values (`PathMatch`), we use OR
> > > match
> > >
> > > Is that correct?
> > > I think if this case happened, we might just bail out directly -- adding
> > > the regex conditions seems unnecessary, we never execute them on Line 127
> > > (but we might still need computing the regex for diagnostics).
> > >
> > > and I'm not sure the motivation of using vector for Conditions and Apply,
> > > it looks like Apply is just 1 element (if we do the bailout for
> > > conditions, Conditions will be at most 1 element).
> >
> > So the explanation for both of these things is that this file is going to
> > grow a lot, and in particular these sections handling PathMatch etc are
> > going to occur again and again, so we need a scalable pattern. I've
> > deliberately limited the scope of the first patchset to only support one
> > condition, one setting to apply etc, but you have to imagine there are
> > likely to be 5 conditions and maybe 25 settings.
> >
> > So with that in mind:
> > - yes, actually saving the conditions isn't necessary, but we do need to
> > evaluate them for side-effects (diagnostics) and it's simpler not to keep
> > track of them. Optimizing performance of broken configs is not a big win :-)
> > - it's marginally simpler to have only one Apply function, but much
> > simpler if separate parts of compile() can push settings independently.
> > Same goes for condition in fact...
> >
> > > Fragment and CompiledFragment is a 1:1 mapping
> >
> > Yes. I think this is somewhere where the naming is helpful at least :-)
> >
> > > A Fragment::Condition represents a single condition, so the AND match
> > > applies for different Fragments, which is in ConfigProvider?
> >
> > The AND match applies to the multiple Conditions in a single
> > CompiledFragment. Each fragment only considers its own conditions.
> > So given:
> > ```
> > If:
> > Platform: win
> > PathMatch: [foo1/.*, foo2/.*]
> > CompileFlags:
> > Add: -foo
> > ---
> > CompileFlags:
> > Add: -bar
> > ```
> >
> > We end up with... one Provider, suppling two CompiledFragments (each
> > compiled from a separate Fragment).
> >
> > The first CompiledFragment has two Conditions, conceptually:
> > - `Params.Platform == "win"`
> > - `Regex("foo1/.*").matches(Params.File) ||
> > Regex("foo2/.*").matches(Params.File)`
> >
> > The second CompiledFragment has no Conditions. Each fragment has one Apply
> > function.
> >
> > The CompiledFragments are applied independently. They each consider their
> > own Conditions, ANDing them together.
> >
> > So CompiledFragment #1 checks both conditions. If any fails, it bails out.
> > Otherwise it runs its Apply function, adding `-foo`.
> >
> > Then CompiledFragment #2 runs. It has no conditions, so never bails out. It
> > adds `-bar`.
> oh, thanks for the explanation, this helps me a lot to understand the code.
>
> so the member `Condition` in `Fragment` will be enhanced to something like
> `std::vector Conditions` in the future? If so, can we defer that for
> `CompiledFragment` as well?
>
>
> Since `Fragment` and `CompiledFragment` is 1:1 mapping, I think it is
> important to keep their implementation aligned, it'd help a lot for readers
> to understand the code. The condition logic (AND, OR) is subtle and
> complicated, I was reading this part of code back and forth, still inferred
> it in an incorrect way.
> so the member Condition in Fragment will be enhanced to something like
> std::vector Conditions in the future?
Ah, not quite...
Fragment is supposed to closely follow the YAML format, which is:
`If: { PathMatc
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall updated this revision to Diff 274623.
sammccall marked 13 inline comments as done.
sammccall added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Rng.emplace();
-Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-Out.Rng->start.character = R.first;
-Out.Rng->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Rng;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
@@ -101,8 +51,8 @@
auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_EQ(Results.size(), 2u);
- EXPECT_FALSE(Results.front().Condition.HasUnrecognizedCondition);
- EXPECT_THAT(Results.front().Condition.PathMatch, ElementsAre(Val("abc")));
+ EXPECT_FALSE(Results.front().If.HasUnrecognizedCondition);
+ EXPECT_THAT(Results.front().If.PathMatch, ElementsAre(Val("abc")));
EXPECT_THAT(Results.front().CompileFlags.Add,
ElementsAre(Val("foo"), Val("bar")));
@@ -120,7 +70,7 @@
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
ASSERT_EQ(Results.size(), 1u);
ASSERT_NE(Results.front().Source.Manager, nullptr);
- EXPECT_EQ(toRange(Results.front().Condition.PathMatch.front().Range,
+ EXPECT_EQ(toRange(Results.front().If.PathMatch.front().Range,
*Results.front().Source.Manager),
YAML.range());
}
@@ -140,7 +90,7 @@
ASSERT_THAT(
Diags.Diagnostics,
- ElementsAre(AllOf(DiagMessage("Unknown Condition key UnknownCondition"),
+ ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"),
DiagKind(llvm::SourceMgr::DK_Warning),
DiagPos(YAML.range().start), DiagRange(YAML.range())),
AllOf(DiagMessage("Unexpected token. Expected Key, Flow "
@@ -150,7 +100,7 @@
ASSERT_EQ(Results.size(), 2u);
EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first")));
- EXPECT_TRUE(Results.front().Condition.HasUnrecognizedCondition);
+ EXPECT_TRUE(Results.front().If.HasUnrecognizedCondition);
EXPECT_THAT(Results.back().CompileFlags.Add, IsEmpty());
}
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -0,0 +1,7
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
hokein added inline comments.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
sammccall wrote:
> hokein wrote:
> > I think if this case happened, we might just bail out directly -- adding
> > the regex conditions seems unnecessary, we never execute them on Line 127
> > (but we might still need computing the regex for diagnostics).
> >
> > and I'm not sure the motivation of using `vector` for `Conditions` and
> > `Apply`, it looks like Apply is just 1 element (if we do the bailout for
> > conditions, `Conditions` will be at most 1 element).
> >
> > I feel like using a single unique_function for `Conditions` and `Apply`
> > might be a better fit with `Fragment`. There is a comment in
> > `ConfigFragment.h`:
> >
> > > Each separate condition must match (combined with AND).
> > > When one condition has multiple values, any may match (combined with OR).
> >
> > so my reading is that:
> >
> > - `Fragment` and `CompiledFragment` is a 1:1 mapping
> > - A `Fragment::Condition` represents a single condition, so the AND match
> > applies for different `Fragment`s, which is in ConfigProvider?
> > - A single condition can have multiple values (`PathMatch`), we use OR match
> >
> > Is that correct?
> > I think if this case happened, we might just bail out directly -- adding
> > the regex conditions seems unnecessary, we never execute them on Line 127
> > (but we might still need computing the regex for diagnostics).
> >
> > and I'm not sure the motivation of using vector for Conditions and Apply,
> > it looks like Apply is just 1 element (if we do the bailout for conditions,
> > Conditions will be at most 1 element).
>
> So the explanation for both of these things is that this file is going to
> grow a lot, and in particular these sections handling PathMatch etc are going
> to occur again and again, so we need a scalable pattern. I've deliberately
> limited the scope of the first patchset to only support one condition, one
> setting to apply etc, but you have to imagine there are likely to be 5
> conditions and maybe 25 settings.
>
> So with that in mind:
> - yes, actually saving the conditions isn't necessary, but we do need to
> evaluate them for side-effects (diagnostics) and it's simpler not to keep
> track of them. Optimizing performance of broken configs is not a big win :-)
> - it's marginally simpler to have only one Apply function, but much simpler
> if separate parts of compile() can push settings independently. Same goes for
> condition in fact...
>
> > Fragment and CompiledFragment is a 1:1 mapping
>
> Yes. I think this is somewhere where the naming is helpful at least :-)
>
> > A Fragment::Condition represents a single condition, so the AND match
> > applies for different Fragments, which is in ConfigProvider?
>
> The AND match applies to the multiple Conditions in a single
> CompiledFragment. Each fragment only considers its own conditions.
> So given:
> ```
> If:
> Platform: win
> PathMatch: [foo1/.*, foo2/.*]
> CompileFlags:
> Add: -foo
> ---
> CompileFlags:
> Add: -bar
> ```
>
> We end up with... one Provider, suppling two CompiledFragments (each compiled
> from a separate Fragment).
>
> The first CompiledFragment has two Conditions, conceptually:
> - `Params.Platform == "win"`
> - `Regex("foo1/.*").matches(Params.File) ||
> Regex("foo2/.*").matches(Params.File)`
>
> The second CompiledFragment has no Conditions. Each fragment has one Apply
> function.
>
> The CompiledFragments are applied independently. They each consider their own
> Conditions, ANDing them together.
>
> So CompiledFragment #1 checks both conditions. If any fails, it bails out.
> Otherwise it runs its Apply function, adding `-foo`.
>
> Then CompiledFragment #2 runs. It has no conditions, so never bails out. It
> adds `-bar`.
oh, thanks for the explanation, this helps me a lot to understand the code.
so the member `Condition` in `Fragment` will be enhanced to something like
`std::vector Conditions` in the future? If so, can we defer that for
`CompiledFragment` as well?
Since `Fragment` and `CompiledFragment` is 1:1 mapping, I think it is important
to keep their implementation aligned, it'd help a lot for readers to understand
the code. The condition logic (AND, OR) is subtle and complicated, I was
reading this part of code back and forth, still inferred it in an incorrect way.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:40
+
+struct Impl {
+ std::vector> Conditions;
nit: I'd use a more verbose name, `CompiledFragmentImpl`.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:46
+ if (!C(P)) {
+dlog("config::Fragment {0}: condition not met", this);
+return false;
maybe `Impl::applying config ...` to allow better
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall updated this revision to Diff 274503.
sammccall added a comment.
CompiledFragment becomes a std::function, compile interface moved to Fragment.h
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Rng.emplace();
-Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-Out.Rng->start.character = R.first;
-Out.Rng->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Rng;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -0,0 +1,77 @@
+//===-- ConfigTesting.h - Helpers for configuration tests ---*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+
+#include "Protocol.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Provides a DiagnosticsCallback that records diganostics.
+// Unlike just pushing them into a vector, underlying storage need not survive.
+struct CapturedDiags {
+ std::function callback() {
+return [this](const llvm::SMDiagnostic &D) {
+ Diagnostics.emplace_back();
+ Diag &Out = Diagnostics.back();
+ Out.Message = D.getMessage().str();
+ Out.Kind = D.getKind();
+ Out.Pos.line = D.getLineNo() - 1;
+ Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+ if (!D.getRanges().empty()) {
+const auto &R = D.getRanges().front();
+Out.Range.emplace();
+Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
+Out.Range->start.character = R.first;
+Out.Range->end.character = R.second;
+ }
+};
+ }
+ struct Diag {
+std::string Message;
+llvm::SourceMgr::DiagKind Kind;
+Position Pos;
+llvm::Optional Range;
+
+friend void PrintTo(const Diag &D, std::ostream *OS) {
+ *OS
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/ConfigProvider.h:47 + /// This always produces a usable compiled fragment (errors are recovered). + explicit CompiledFragment(Fragment, DiagnosticCallback); + sammccall wrote: > hokein wrote: > > sammccall wrote: > > > hokein wrote: > > > > Would it be more nature to have a function `compile`(or so) to do the > > > > actual compile (Fragment -> CompiledFragment) rather than doing it in a > > > > constructor? > > > I'm not sure. The idea that the input/result is > > > Fragment/CompiledFragment, and that the compilation cannot fail, suggests > > > to me that a constructor is OK here. > > > On the other hand, the DiangosticCallback param (which we don't keep a > > > reference to) is a bit weird. > > > > > > So I don't really feel strongly about it one way or the other, happy to > > > change it if you do think it would be significantly better. > > You point is fair. > > I'm not sure using `compile` is *significantly* better, but it is better > > IMO -- conceptually, the class name `CompiledFragment` implies that there > > should be a `compile` API (this was my impression when reading the code at > > the first time) > > > > it might be ok to keep as-is. > Argh, you convinced me this would be better, but I can't come up with a form > I like. > > - `static CompiledFragment::compile` repeats itself in a weird way. But any > other option requires messing around with friend functions, which feels off > - `Fragment::compile` is maybe the most natural, but the dependencies are > backwards (needs the CompiledFragment type, it gets implemented in the wrong > file...). > - standalone `compile()` is maybe the most palatable, but seems a little > undiscoverable > - having a function return CompiledFragment means that putting it into a > shared_ptr is annoying work, and breaks our pointer-logging trick, but having > the factory function return a pointer also seems odd. > > I'm starting to think maybe the CompiledFragment class shouldn't be public at > all, and we should just `using CompiledFragment = unique_function Params&, Config&) const>`. WDYT? > I'm starting to think maybe the CompiledFragment class shouldn't be public at > all Thinking more about this: - making it std::function<...> instead lets us wrap the shared_ptr inside which makes a few APIs a bit cleaner - as its an interface type, this leaves config providers free to use other ways of manipulating config rather than going via fragments, which is potentially nice for embedders - the main downside is we're committing to not exposing any more structure (e.g. querying a fragment for which directory it applies to) starting to quite like this idea. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82612/new/ https://reviews.llvm.org/D82612 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall updated this revision to Diff 274470.
sammccall marked an inline comment as done.
sammccall added a comment.
address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Rng.emplace();
-Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-Out.Rng->start.character = R.first;
-Out.Rng->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Rng;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -0,0 +1,77 @@
+//===-- ConfigTesting.h - Helpers for configuration tests ---*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+
+#include "Protocol.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Provides a DiagnosticsCallback that records diganostics.
+// Unlike just pushing them into a vector, underlying storage need not survive.
+struct CapturedDiags {
+ std::function callback() {
+return [this](const llvm::SMDiagnostic &D) {
+ Diagnostics.emplace_back();
+ Diag &Out = Diagnostics.back();
+ Out.Message = D.getMessage().str();
+ Out.Kind = D.getKind();
+ Out.Pos.line = D.getLineNo() - 1;
+ Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+ if (!D.getRanges().empty()) {
+const auto &R = D.getRanges().front();
+Out.Range.emplace();
+Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
+Out.Range->start.character = R.first;
+Out.Range->end.character = R.second;
+ }
+};
+ }
+ struct Diag {
+std::string Message;
+llvm::SourceMgr::DiagKind Kind;
+Position Pos;
+llvm::Optional Range;
+
+friend void PrintTo(const Diag &D, std::ostream *OS) {
+ *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall marked 8 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+ ConfigCompile.cpp
ConfigYAML.cpp
Diagnostics.cpp
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > we have a few config-related files now, I wonder would it make sense to
> > > create a config/ subdir?
> > It seems marginal to me - could go either way. We have a small handful (2
> > headers and 3 impl files). There may be 1-2 more, but I'm not sure yet.
> > (config-files-on-disk, and json->fragment).
> > So 6-7 files... smaller than the other subdirs, but not tiny.
> >
> > There's also the fact that config.h/cpp is not like the others: all
> > features (not just infra) depend on this, it's not in the `config::`
> > namespace etc. So should it really be in the subdir?
> >
> > Mind if we hold off on this until at least the scope of D82335 has landed?
> > I think it would be awkward to do this move halfway through this patchset,
> > but fairly easy to do at any point that things are stable.
> > it seems marginal to me - could go either way. We have a small handful (2
> > headers and 3 impl files). There may be 1-2 more, but I'm not sure yet.
> > (config-files-on-disk, and json->fragment).
> > So 6-7 files... smaller than the other subdirs, but not tiny.
>
> if we end up with 6-7 files, I'd prefer to create a subdirectory, which make
> our source structure a bit tidy, our clangd/ dir already has a lot of source
> files.
>
>
>
> > Mind if we hold off on this until at least the scope of D82335 has landed?
> > I think it would be awkward to do this move halfway through this patchset,
> > but fairly easy to do at any point that things are stable.
>
> sure, I didn't mean we should do this in this patch.
> if we end up with 6-7 files, I'd prefer to create a subdirectory
SGTM. I'll carry on with this structure for now, and move files after - please
remind me if I forget!
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
hokein wrote:
> I think if this case happened, we might just bail out directly -- adding the
> regex conditions seems unnecessary, we never execute them on Line 127 (but we
> might still need computing the regex for diagnostics).
>
> and I'm not sure the motivation of using `vector` for `Conditions` and
> `Apply`, it looks like Apply is just 1 element (if we do the bailout for
> conditions, `Conditions` will be at most 1 element).
>
> I feel like using a single unique_function for `Conditions` and `Apply` might
> be a better fit with `Fragment`. There is a comment in `ConfigFragment.h`:
>
> > Each separate condition must match (combined with AND).
> > When one condition has multiple values, any may match (combined with OR).
>
> so my reading is that:
>
> - `Fragment` and `CompiledFragment` is a 1:1 mapping
> - A `Fragment::Condition` represents a single condition, so the AND match
> applies for different `Fragment`s, which is in ConfigProvider?
> - A single condition can have multiple values (`PathMatch`), we use OR match
>
> Is that correct?
> I think if this case happened, we might just bail out directly -- adding the
> regex conditions seems unnecessary, we never execute them on Line 127 (but we
> might still need computing the regex for diagnostics).
>
> and I'm not sure the motivation of using vector for Conditions and Apply, it
> looks like Apply is just 1 element (if we do the bailout for conditions,
> Conditions will be at most 1 element).
So the explanation for both of these things is that this file is going to grow
a lot, and in particular these sections handling PathMatch etc are going to
occur again and again, so we need a scalable pattern. I've deliberately limited
the scope of the first patchset to only support one condition, one setting to
apply etc, but you have to imagine there are likely to be 5 conditions and
maybe 25 settings.
So with that in mind:
- yes, actually saving the conditions isn't necessary, but we do need to
evaluate them for side-effects (diagnostics) and it's simpler not to keep track
of them. Optimizing performance of broken configs is not a big win :-)
- it's marginally simpler to have only one Apply function, but much simpler if
separate parts of compile() can push settings independently. Same goes for
condition in fact...
> Fragment and CompiledFragment is a 1:1 mapping
Yes. I think this is somewhere where the naming is helpful at least :-)
> A Fragment::Condition represents a single condition, so the AND match applies
> for different Fragments, which is in ConfigProvider?
The AND match applies to the multiple Conditions in a single CompiledFragment.
Each fragment only considers its own conditions.
So given:
```
If:
Platform: win
PathMatch: [foo1/.*, foo2/.*]
CompileFlags
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
hokein added inline comments.
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+ ConfigCompile.cpp
ConfigYAML.cpp
Diagnostics.cpp
sammccall wrote:
> hokein wrote:
> > we have a few config-related files now, I wonder would it make sense to
> > create a config/ subdir?
> It seems marginal to me - could go either way. We have a small handful (2
> headers and 3 impl files). There may be 1-2 more, but I'm not sure yet.
> (config-files-on-disk, and json->fragment).
> So 6-7 files... smaller than the other subdirs, but not tiny.
>
> There's also the fact that config.h/cpp is not like the others: all features
> (not just infra) depend on this, it's not in the `config::` namespace etc. So
> should it really be in the subdir?
>
> Mind if we hold off on this until at least the scope of D82335 has landed? I
> think it would be awkward to do this move halfway through this patchset, but
> fairly easy to do at any point that things are stable.
> it seems marginal to me - could go either way. We have a small handful (2
> headers and 3 impl files). There may be 1-2 more, but I'm not sure yet.
> (config-files-on-disk, and json->fragment).
> So 6-7 files... smaller than the other subdirs, but not tiny.
if we end up with 6-7 files, I'd prefer to create a subdirectory, which make
our source structure a bit tidy, our clangd/ dir already has a lot of source
files.
> Mind if we hold off on this until at least the scope of D82335 has landed? I
> think it would be awkward to do this move halfway through this patchset, but
> fairly easy to do at any point that things are stable.
sure, I didn't mean we should do this in this patch.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:65
+if (F.HasUnrecognizedCondition)
+ Conditions.push_back([&](const Params &) { return false; });
+
I think if this case happened, we might just bail out directly -- adding the
regex conditions seems unnecessary, we never execute them on Line 127 (but we
might still need computing the regex for diagnostics).
and I'm not sure the motivation of using `vector` for `Conditions` and `Apply`,
it looks like Apply is just 1 element (if we do the bailout for conditions,
`Conditions` will be at most 1 element).
I feel like using a single unique_function for `Conditions` and `Apply` might
be a better fit with `Fragment`. There is a comment in `ConfigFragment.h`:
> Each separate condition must match (combined with AND).
> When one condition has multiple values, any may match (combined with OR).
so my reading is that:
- `Fragment` and `CompiledFragment` is a 1:1 mapping
- A `Fragment::Condition` represents a single condition, so the AND match
applies for different `Fragment`s, which is in ConfigProvider?
- A single condition can have multiple values (`PathMatch`), we use OR match
Is that correct?
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:84
+ void compile(Fragment::CompileFlagsBlock &&F) {
+if (!F.Add.empty()) {
+ std::vector Add;
nit: use early return.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+ constexpr static auto Error = llvm::SourceMgr::DK_Error;
+ void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
sammccall wrote:
> hokein wrote:
> > this member seems unused.
> it's used on line 52 `diag(Error, ...)`.
>
> I wouldn't bother extracting it except there should be a bunch more places
> where errors are emitted, and keeping them readable is worthwhile IME.
Ah, I see. I must have missed that.
btw, could you spell out the auto type for `Error`? I believe it is `DiagKind`,
and it would be more clear that it is used in the first parameter of `diag`
(given that they are close).
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+ /// Absolute path to file we're targeting. Unix slashes.
sammccall wrote:
> hokein wrote:
> > nit: I find the name `Params` is too general, it implies less information.
> > When I read the code using `Params`, I have to go-to-definition to see what
> > it means.
> >
> > if we only have a single member in this struct, maybe just use it directly.
> Agree with the vague name.
> In most contexts, the name is `config::Params` which is at least a bit more
> specific.
>
> > if we only have a single member in this struct, maybe just use it directly.
>
> I don't like this, though - there are likely to be more things here in the
> future (e.g. platform), and passing around a string is much less clear as to
> intent. So any idea about a name for this struct?
>
> What about `config::Environment` or `config::Env`? still too vague?
not better idea. `config::Env` doesn't seem to be much better... and the
signature `apply(const Params &P, Config &C)` seems neat
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall marked 10 inline comments as done.
sammccall added inline comments.
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+ ConfigCompile.cpp
ConfigYAML.cpp
Diagnostics.cpp
hokein wrote:
> we have a few config-related files now, I wonder would it make sense to
> create a config/ subdir?
It seems marginal to me - could go either way. We have a small handful (2
headers and 3 impl files). There may be 1-2 more, but I'm not sure yet.
(config-files-on-disk, and json->fragment).
So 6-7 files... smaller than the other subdirs, but not tiny.
There's also the fact that config.h/cpp is not like the others: all features
(not just infra) depend on this, it's not in the `config::` namespace etc. So
should it really be in the subdir?
Mind if we hold off on this until at least the scope of D82335 has landed? I
think it would be awkward to do this move halfway through this patchset, but
fairly easy to do at any point that things are stable.
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+ constexpr static auto Error = llvm::SourceMgr::DK_Error;
+ void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
hokein wrote:
> this member seems unused.
it's used on line 52 `diag(Error, ...)`.
I wouldn't bother extracting it except there should be a bunch more places
where errors are emitted, and keeping them readable is worthwhile IME.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+ /// Absolute path to file we're targeting. Unix slashes.
hokein wrote:
> nit: I find the name `Params` is too general, it implies less information.
> When I read the code using `Params`, I have to go-to-definition to see what
> it means.
>
> if we only have a single member in this struct, maybe just use it directly.
Agree with the vague name.
In most contexts, the name is `config::Params` which is at least a bit more
specific.
> if we only have a single member in this struct, maybe just use it directly.
I don't like this, though - there are likely to be more things here in the
future (e.g. platform), and passing around a string is much less clear as to
intent. So any idea about a name for this struct?
What about `config::Environment` or `config::Env`? still too vague?
Comment at: clang-tools-extra/clangd/ConfigProvider.h:40
+///
+/// Fragments are compiled by Providers when first loaded, and cached for
reuse.
+/// Like a compiled program, this is good for performance and also encourages
hokein wrote:
> I might miss the context, where is the provider, not added yet?
Right, D82335 has a rought draft of everything together (it's called
ConfigProvider in that patch).
I think it's less churn-y to add the documentation as the classes are added,
even if it means temporarily referring to something that doesn't exist yet.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+ /// This always produces a usable compiled fragment (errors are recovered).
+ explicit CompiledFragment(Fragment, DiagnosticCallback);
+
hokein wrote:
> Would it be more nature to have a function `compile`(or so) to do the actual
> compile (Fragment -> CompiledFragment) rather than doing it in a constructor?
I'm not sure. The idea that the input/result is Fragment/CompiledFragment, and
that the compilation cannot fail, suggests to me that a constructor is OK here.
On the other hand, the DiangosticCallback param (which we don't keep a
reference to) is a bit weird.
So I don't really feel strongly about it one way or the other, happy to change
it if you do think it would be significantly better.
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:40
+TEST_F(ConfigCompileTests, Condition) {
+ // No condition.
+ Frag.CompileFlags.Add.emplace_back("X");
hokein wrote:
> my first impression was that each `// XXX` is a separated test, but it turns
> out not, the tests here seem to be stateful -- `Frag` keeps being modified. I
> think it is intentional, but it is hard for readers to track given that the
> code is long.
Agree. Made this test reset fragment state after each group of assertions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall updated this revision to Diff 274237.
sammccall marked 3 inline comments as done.
sammccall added a comment.
Address some review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Rng.emplace();
-Out.Rng->start.line = Out.Rng->end.line = Out.Pos.line;
-Out.Rng->start.character = R.first;
-Out.Rng->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Rng;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Rng == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -0,0 +1,77 @@
+//===-- ConfigTesting.h - Helpers for configuration tests ---*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+
+#include "Protocol.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Provides a DiagnosticsCallback that records diganostics.
+// Unlike just pushing them into a vector, underlying storage need not survive.
+struct CapturedDiags {
+ std::function callback() {
+return [this](const llvm::SMDiagnostic &D) {
+ Diagnostics.emplace_back();
+ Diag &Out = Diagnostics.back();
+ Out.Message = D.getMessage().str();
+ Out.Kind = D.getKind();
+ Out.Pos.line = D.getLineNo() - 1;
+ Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+ if (!D.getRanges().empty()) {
+const auto &R = D.getRanges().front();
+Out.Range.emplace();
+Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
+Out.Range->start.character = R.first;
+Out.Range->end.character = R.second;
+ }
+};
+ }
+ struct Diag {
+std::string Message;
+llvm::SourceMgr::DiagKind Kind;
+Position Pos;
+llvm::Optional Range;
+
+friend void PrintTo(const Diag &D, std::ostream *OS) {
+ *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error:
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
hokein added inline comments.
Comment at: clang-tools-extra/clangd/CMakeLists.txt:41
+ ConfigCompile.cpp
ConfigYAML.cpp
Diagnostics.cpp
we have a few config-related files now, I wonder would it make sense to create
a config/ subdir?
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:47
+
+ std::string RegexError = "";
+ llvm::Optional compileRegex(const Located &Text) {
this is only used in method `compileRegex`, looks like can be moved to a local
variable? is there life-time issue?
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:96
+
+ constexpr static auto Error = llvm::SourceMgr::DK_Error;
+ void diag(llvm::SourceMgr::DiagKind Kind, llvm::StringRef Message,
this member seems unused.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:32
+/// Describes the context used to evaluate configuration fragments.
+struct Params {
+ /// Absolute path to file we're targeting. Unix slashes.
nit: I find the name `Params` is too general, it implies less information. When
I read the code using `Params`, I have to go-to-definition to see what it
means.
if we only have a single member in this struct, maybe just use it directly.
Comment at: clang-tools-extra/clangd/ConfigProvider.h:40
+///
+/// Fragments are compiled by Providers when first loaded, and cached for
reuse.
+/// Like a compiled program, this is good for performance and also encourages
I might miss the context, where is the provider, not added yet?
Comment at: clang-tools-extra/clangd/ConfigProvider.h:47
+ /// This always produces a usable compiled fragment (errors are recovered).
+ explicit CompiledFragment(Fragment, DiagnosticCallback);
+
Would it be more nature to have a function `compile`(or so) to do the actual
compile (Fragment -> CompiledFragment) rather than doing it in a constructor?
Comment at: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp:40
+TEST_F(ConfigCompileTests, Condition) {
+ // No condition.
+ Frag.CompileFlags.Add.emplace_back("X");
my first impression was that each `// XXX` is a separated test, but it turns
out not, the tests here seem to be stateful -- `Frag` keeps being modified. I
think it is intentional, but it is hard for readers to track given that the
code is long.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82612/new/
https://reviews.llvm.org/D82612
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config
sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov, mgorny.
Herald added a project: clang.
sammccall added a parent revision: D82606: [clangd] Config: config struct
propagated through Context.
sammccall edited the summary of this revision.
sammccall added a reviewer: hokein.
https://reviews.llvm.org/D82335 shows how this fits in with related config work.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82612
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigProvider.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigTesting.h
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -8,14 +8,13 @@
#include "Annotations.h"
#include "ConfigFragment.h"
-#include "Matchers.h"
+#include "ConfigTesting.h"
#include "Protocol.h"
#include "llvm/Support/SMLoc.h"
#include "llvm/Support/ScopedPrinter.h"
#include "llvm/Support/SourceMgr.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#include "gtest/internal/gtest-internal.h"
namespace clang {
namespace clangd {
@@ -36,55 +35,6 @@
return false;
}
-Position toPosition(llvm::SMLoc L, const llvm::SourceMgr &SM) {
- auto LineCol = SM.getLineAndColumn(L);
- Position P;
- P.line = LineCol.first - 1;
- P.character = LineCol.second - 1;
- return P;
-}
-
-Range toRange(llvm::SMRange R, const llvm::SourceMgr &SM) {
- return Range{toPosition(R.Start, SM), toPosition(R.End, SM)};
-}
-
-struct CapturedDiags {
- std::function callback() {
-return [this](const llvm::SMDiagnostic &D) {
- Diagnostics.emplace_back();
- Diag &Out = Diagnostics.back();
- Out.Message = D.getMessage().str();
- Out.Kind = D.getKind();
- Out.Pos.line = D.getLineNo() - 1;
- Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
- if (!D.getRanges().empty()) {
-const auto &R = D.getRanges().front();
-Out.Range.emplace();
-Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
-Out.Range->start.character = R.first;
-Out.Range->end.character = R.second;
- }
-};
- }
- struct Diag {
-std::string Message;
-llvm::SourceMgr::DiagKind Kind;
-Position Pos;
-llvm::Optional Range;
-
-friend void PrintTo(const Diag &D, std::ostream *OS) {
- *OS << (D.Kind == llvm::SourceMgr::DK_Error ? "error: " : "warning: ")
- << D.Message << "@" << llvm::to_string(D.Pos);
-}
- };
- std::vector Diagnostics;
-};
-
-MATCHER_P(DiagMessage, M, "") { return arg.Message == M; }
-MATCHER_P(DiagKind, K, "") { return arg.Kind == K; }
-MATCHER_P(DiagPos, P, "") { return arg.Pos == P; }
-MATCHER_P(DiagRange, R, "") { return arg.Range == R; }
-
TEST(ParseYAML, SyntacticForms) {
CapturedDiags Diags;
const char *YAML = R"yaml(
Index: clang-tools-extra/clangd/unittests/ConfigTesting.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/ConfigTesting.h
@@ -0,0 +1,77 @@
+//===-- ConfigTesting.h - Helpers for configuration tests ---*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_UNITTESTS_CONFIGTESTING_H
+
+#include "Protocol.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock.h"
+#include
+
+namespace clang {
+namespace clangd {
+namespace config {
+
+// Provides a DiagnosticsCallback that records diganostics.
+// Unlike just pushing them into a vector, underlying storage need not survive.
+struct CapturedDiags {
+ std::function callback() {
+return [this](const llvm::SMDiagnostic &D) {
+ Diagnostics.emplace_back();
+ Diag &Out = Diagnostics.back();
+ Out.Message = D.getMessage().str();
+ Out.Kind = D.getKind();
+ Out.Pos.line = D.getLineNo() - 1;
+ Out.Pos.character = D.getColumnNo(); // Zero-based - bug in SourceMgr?
+ if (!D.getRanges().empty()) {
+const auto &R = D.getRanges().front();
+Out.Range.emplace();
+Out.Range->start.line = Out.Range->end.line = Out.Pos.line;
+Out.Range->start.character = R.first;
+Out.Range->end.character = R.second;
+ }
+};
+ }
+ str
