courbet marked an inline comment as done.
courbet added inline comments.
================
Comment at: test/SemaCXX/static-assert.cpp:111
+static_assert(std::is_same<ExampleTypes::T, ExampleTypes::U>::value,
"message"); // expected-error{{static_assert failed due to requirement
'std::is_same<int, float>::value' "message"}}
+static_assert(std::is_const<ExampleTypes::T>::value, "message");
// expected-error{{static_assert failed due to requirement
'std::is_const<int>::value' "message"}}
----------------
Quuxplusone wrote:
> aaron.ballman wrote:
> > courbet wrote:
> > > Quuxplusone wrote:
> > > > I would like to see some more realistic test cases. I suggest this test
> > > > case for example:
> > > > ```
> > > > struct BI_tag {};
> > > > struct RAI_tag : BI_tag {};
> > > > struct MyIterator {
> > > > using tag = BI_tag;
> > > > };
> > > > struct MyContainer {
> > > > using iterator = MyIterator;
> > > > };
> > > > template<class Container>
> > > > void foo() {
> > > > static_assert(std::is_base_of_v<RAI_tag, typename
> > > > Container::iterator::tag>);
> > > > }
> > > > ```
> > > > This is an example where as a programmer I would not want to see
> > > > //only// `failed due to requirement std::is_base_of_v<RAI_tag, BI_tag>`
> > > > — that doesn't help me solve the issue. OTOH, since every diagnostic
> > > > includes a cursor to the exact text of the `static_assert` already, I
> > > > think it's fair to say that the current diagnostic message is
> > > > redundant, and therefore it's okay to replace it (as you propose to do)
> > > > with something that is not redundant.
> > > > I think it's fair to say that the current diagnostic message is
> > > > redundant, and therefore it's okay to replace it (as you propose to do)
> > > > with something that is not redundant.
> > >
> > > Yes, the proposal here might not be the *best* possible diagnostic for
> > > all cases, but it's already a huge improvement on the existing one, and
> > > solves a significant proportion of use cases.
> > >
> > > Here, the programmer will see:
> > > ```
> > > test.cc:13:5: error: static_assert failed due to requirement
> > > 'std::is_base_of<RAI_tag, BI_tag>::value'
> > > static_assert(std::is_base_of<RAI_tag, typename
> > > Container::iterator::tag>::value);
> > > ^
> > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > ```
> > > which I think is a reasonable help for debugging.
> > >
> > @Quuxplusone, do you have recommendations for what you'd prefer to see
> > instead?
> >
> > FWIW, I think this is a good incremental improvement. If there's more
> > information we could display easily as part of this patch, we should
> > consider it, but I'm also fine with saying this is progress.
> > @Quuxplusone, do you have recommendations for what you'd prefer to see
> > instead?
>
> On the diagnostic itself, no, this looks good and I was just thinking out
> loud.
>
> On the test cases, yes, I suggest that there should be at least one test case
> where a `static_assert` appears inside a template and uses something
> template-dependent.
SG, I'll add such an example (with std::is_same if you don't mind to avoid
having to duplicate the whole `<type_traits>` in the test :) )
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54903/new/
https://reviews.llvm.org/D54903
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
