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

Mon Jul 31 12:57:19 PDT 2023

mstorsjo added a comment.

In D156573#4547076 <https://reviews.llvm.org/D156573#4547076>, @andrewng wrote:

> 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.

Yep, there is, but most of that is kinda static content (and doesn't interact with the cmake configuration options like this). OTOH I guess it could be possible to merge them into one and have most of the logic be controlled by cmake variables, but that's kinda bending the design of the current config file based test setup, so I'd prefer not to wander into that right now, but keeping the scope limited.

>> 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.

Ok, I'll rebase it that way.

> 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!

No problem, I was interested in getting this fixed anyway - and it's kinda easy when having some idea of how I wanted to get it done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156573