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
