In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

• Remove clean rule
• Use more readable names

In D66447#1636456, @jblachly wrote:

Thank you for creating a revision and reviewing this.

I made inline comments on the test harness and Dlang types / qualifiers.

With removal of the Dlang types, where is the appropriate place to put them?
It is not clear to me whether language plugins can replace the functionality in ClangASTContext::GetBuiltinTypeForDWARFEncodingAndBitSize.

Thanks again

• Address Pavel's comments.

LGTM

In D66429#1636655, @labath wrote:

That said, I'm not a fan of force-overwriting cache variables specified by the user, particularly when the same effect can be achieved by *deleting* the cache variable instead of setting it to an empty string. I think you should be able to reset the default value of LLDB_TEST_C_COMPILER (just like any other cmake variable) by running cmake -ULLDB_TEST_C_COMPILER. You can even delete both variables at once with -ULLDB_TEST_*_COMPILER.

Thanks, Ismail. LGTM!

• Feedback Greg.
• Remove dlang types.

Thanks, Jan. This LGTM with Pavel's comments addressed.

This code review seems to address a few things. I think it might have been easier to discuss if they were separate patches, but here are my comments:

Chris is correct, PYTHON_EXECUTABLE will be an absolute path. For this to work you'll need to remove /usr/bin/env.

Make RegularExpression POSIX compliant.

LGTM with the inline comment.

In D66174#1631055, @labath wrote:

I was unhappy with the () workaround, so I did some investigation. Now, I am even more unhappy, but at least better educated. :P

Basically, what I've learned is that according to POSIX https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap09.html an empty string is not a valid regular expression. Most tools seem to ignore that, and treat it as if it matches everything (and I can't say I blame them). However, the BSD implementation (that the llvm stuff is based on) tried to do a strict implementation and it rejects that as an invalid expression.

Funnily enough, () is not valid according to POSIX either, but the BSD implementation accepts it. So does (linux) grep and python. However, perl rejects it (it still considers it a valid expression, but one that doesn't match anything).

a||b (a OR empty pattern OR b) is also not valid. grep, python and perl accept that, but the BSD regexes don't.

So, overall, it seems to me that there is a lot of confusion about what should empty (sub-)patterns do. I don't think special-casing "" in lldb_private::RegularExpression would help alleviate any of that confusion as we would be still left with all of the other inconsistencies. So, if we want to go through with this (and I still think we should), I guess we'll just have to bite the bullet and say that our expressions are now (more or less) POSIX conformant, and repeat that to anyone who comes complaining that lldb regexes behave differently than grep, python or older versions of lldb...

In D66061#1630825, @CodaFi wrote:

Y’all can revert it. I’ll resubmit the patch with updated tests tomorrow.

This test seems to be failing pretty much everywhere. Can you please have a look or should we revert it?