Page MenuHomePhabricator

[Sema] Simplfy static_assert diagnostic code.
Needs ReviewPublic

Authored by courbet on Thu, Dec 20, 7:38 AM.

Details

Summary

We can now remove the specific handling of NestedNameQualifiers as
it's now handled by the type printer as of r349729.

Diff Detail

Event Timeline

courbet created this revision.Thu, Dec 20, 7:38 AM
aaron.ballman added inline comments.Fri, Dec 21, 11:24 AM
test/SemaCXX/static-assert.cpp
130

Any idea why the std:: was dropped here?

courbet marked an inline comment as done.Wed, Jan 2, 1:33 AM
courbet added inline comments.
test/SemaCXX/static-assert.cpp
130

NestedNameSpecifier::print() explicitly does:

PrintingPolicy InnerPolicy(Policy);
InnerPolicy.SuppressScope = true;
aaron.ballman added inline comments.Wed, Jan 2, 7:48 AM
test/SemaCXX/static-assert.cpp
130

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.

courbet added inline comments.Wed, Jan 2, 8:26 AM
test/SemaCXX/static-assert.cpp
130

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.

aaron.ballman added inline comments.Wed, Jan 2, 8:32 AM
test/SemaCXX/static-assert.cpp
130

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

courbet marked an inline comment as done.Thu, Jan 3, 3:52 AM
courbet added inline comments.
test/SemaCXX/static-assert.cpp
130

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) {
aaron.ballman added inline comments.Thu, Jan 3, 9:10 AM
test/SemaCXX/static-assert.cpp
130

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.