[PATCH] D82612: [clangd] Config: compile Fragment -> CompiledFragment -> Config

[Sam McCall via Phabricator via cfe-commits]

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<Located<std::string>> 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