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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
