aaron.ballman added inline comments.
================
Comment at: test/SemaCXX/static-assert.cpp:130
static_assert(std::is_same<decltype(std::is_const<const ExampleTypes::T>()),
int>::value, "message");
-// expected-error@-1{{static_assert failed due to requirement
'std::is_same<std::is_const<const int>, int>::value' "message"}}
+// expected-error@-1{{static_assert failed due to requirement
'std::is_same<is_const<const int>, int>::value' "message"}}
static_assert(std::is_const<decltype(ExampleTypes::T(3))>::value, "message");
----------------
courbet wrote:
> aaron.ballman wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > courbet wrote:
> > > > > aaron.ballman wrote:
> > > > > > Any idea why the `std::` was dropped here?
> > > > > `NestedNameSpecifier::print()` explicitly does:
> > > > >
> > > > > ```
> > > > > PrintingPolicy InnerPolicy(Policy);
> > > > > InnerPolicy.SuppressScope = true;
> > > > > ```
> > > > >
> > > > Ah, good point, but is that a good behavioral change? I slightly prefer
> > > > printing the namespace name there -- it will likely be redundant
> > > > information most of the time, but when the namespace actually matters,
> > > > having it printed could save someone a lot of head scratching.
> > > > I slightly prefer printing the namespace name there
> > >
> > > I tend to agree, so it's more a trade-off of code complexity vs better
> > > diagnostic - I tend to err on the side of simplifying the code :)
> > >
> > > Another option is to add yet another boolean to PrintingPolicy, but I
> > > htink this is too narrow a use case.
> > Heh, I tend to err on the side of helping the user unless the code will be
> > truly awful. I agree that another option on PrintingPolicy may not be the
> > best approach. Do you know why the namespace is being suppressed in the
> > first place? Another option would be to always print the namespace, but I
> > don't know what that might regress (if anything).
> Unfortunately always printing the qualification breaks 30 tests, including
> some in a very bad way:
>
> ```
> /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/AST/ast-print-out-of-line-func.cpp:29:11:
> error: CHECK: expected string not found in input
> // CHECK: void Wrapper::Inner::operator+=(int)
> ^
> <stdin>:14:43: note: scanning from here
> ns::Wrapper::ns::Wrapper::Inner::Inner() {
> ^
> <stdin>:16:19: note: possible intended match here
> void ns::Wrapper::ns::Wrapper::Inner::operator+=(int) {
> ```
That looks like a bug with the printer, no?
As it stands, I think this is a regression with the diagnostic printing. I
think it would be reasonable if the namespace were dropped only when the
namespace reduces clarity (e.g., there are no other conflicting names in other
namespaces, so the namespace is superfluous), but I don't think that's what's
happening here.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55932/new/
https://reviews.llvm.org/D55932
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
