[libcxx-commits] [PATCH] D156573: [libcxx] [test] Deduplicate setting parameters for clangcl test configs. NFC.

[Andrew Ng via Phabricator via libcxx-commits]

andrewng accepted this revision.
andrewng added a comment.

LGTM and is definitely an improvement. However, there's still quite a bit of duplicated content. Perhaps the templates could be merged and the other differences put into variables? Although that can be a separate patch.

> Any preference on landing (and adapting) this to go in before the two dependencies (D155561 <https://reviews.llvm.org/D155561> and D155562 <https://reviews.llvm.org/D155562>), or keeping it where it is now afterwards? I can adapt it so this goes in first too. The second one of those dependencies isn’t approved yet (and depends on the requirement being raised to clang 16, which I guess we can do now that the branch is made - but holding off of that for a while might ease backporting).

I think it would be nice to get this in before D155562 <https://reviews.llvm.org/D155562>, just to avoid the duplication within the patch itself. But I don't think it's a requirement.

By the way, thanks for breaking up my patch and getting most of it submitted. I wouldn't have been able to find the time to do it for quite a while!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156573/new/

https://reviews.llvm.org/D156573