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.

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to