[PATCH] D105127: Implement P1401R5

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov requested changes to this revision.
mizvekov added a comment.
This revision now requires changes to proceed.

So I read the paper, downloaded this patch, played around with it a little bit, tried some different tests, like expressions with dependent types,
classes with regular/explicit user-defined conversions, function names and some other examples that are mentioned in the paper.
It works fine on these.

However, I confirm that the failures in `CXX/except/except.spec/p1.cpp` detected by the buildbots are real.
It fails for me with this DR, works on parent revision.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:1491
+def err_constexpr_if_condition_expression_is_not_constant : Error<
+  "constexpr if condition is not a constant expression convertible to bool">;
 def err_static_assert_failed : Error<"static_assert failed%select{ %1|}0">;
----------------
cor3ntin wrote:
> mizvekov wrote:
> > Looks a bit easier to parse the english there.
> I would rather not change that, to remain consistent with existing diagnostics involving `constexpr if`
> But I agree it might be good to change them all
I see, yeah agreed.


================
Comment at: clang/test/CXX/stmt.stmt/stmt.select/stmt.if/p2.cpp:43
 namespace ccce {
+struct S {
+} s;
----------------
cor3ntin wrote:
> mizvekov wrote:
> > This is not consistently indented.
> Unfortunately, it was put there by clang-format, should I move it manually?
In general you should respect the formatting tips you get from the non-test code, as these will fail the pre-merge checks.
But in the test code, our buildbot produces the clang-format patch but this is not really enforced when merging.

I think what you are doing here, keeping the existing style, is reasonable so you should disregard this particular tip from the format patch.
The other option would be to format everything in a pre-work NFC commit. This should be fine for pure semantic tests like these, but you have to be careful with parser tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105127