[Sema] Better static assert diagnostics for expressions involving temporaries.

[courbet]

Summary

Handles expressions such as:

• std::is_const<T>()
• std::is_const<T>()();
• std::is_same(decltype(U()), V>::value;

[courbet]

cosmetics

[Quuxplusone]

lib/Sema/SemaTemplate.cpp

3089 ↗

(On Diff #177681)

It might be more maintainer-proof to write this as

std::pair<const char *, const char *> Braces;
if (Node->isStdInitListInitialization())
  Braces = std::make_pair("", "");
else if (Node->isListInitialization())
  Braces = std::make_pair("{", "}");
else
  Braces = std::make_pair("(", ")");
OS << Braces.first;
for (...) { ... }
OS << Braces.second;
return true;

test/SemaCXX/static-assert.cpp

136 ↗

(On Diff #177681)

Conspicuously missing any test for lines 3081–3106 above. IIUC, those lines would trigger on things like

template<class T> struct X { int i=0, j=0; constexpr operator bool() const { return false; } };
template<class T> void test() {
    static_assert(X<T>{});
    static_assert(X<T>{1,2});
    static_assert(T{0});
    static_assert(T(0));
}
template void test<int>();

But I guess I don't see why extra code is needed to handle those; shouldn't the pretty-printer handle them already? What do the current diagnostics look like for my example?

[courbet]

test/SemaCXX/static-assert.cpp

136 ↗

(On Diff #177681)

Note: Your examples are CXXFunctionalCastExpr, and CXXUnresolvedConstructExpr respectively, not CXXTemporaryObjectExpr, so they are not handled by this change.

They are correctly printed before this change with T==int.

The issue comes when adding an extra layer of indirection:

struct ExampleTypes {
  explicit ExampleTypes(int);
  using T = int;
  using U = float;
};

template<class T> struct X {
  int i=0;
  int j=0;
  constexpr operator bool() const { return false; }
};

template<class T> void foo6() {
    static_assert(X<typename T::T>());
    static_assert(X<typename T::T>{});
    static_assert(X<typename T::T>{1,2});
    static_assert(X<typename T::T>({1,2}));
    static_assert(typename T::T{0});
    static_assert(typename T::T(0));
}
template void foo6<ExampleTypes>();

The errors before the change are:

File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 71: static_assert failed due to requirement 'X<typename ExampleTypes::T>()'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 72: static_assert failed due to requirement 'X<typename ExampleTypes::T>{}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 73: static_assert failed due to requirement 'X<typename ExampleTypes::T>{1, 2}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 74: static_assert failed due to requirement 'X<typename ExampleTypes::T>({1, 2})'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 75: static_assert failed due to requirement 'typename ExampleTypes::T{0}'
  File /usr/local/google/home/courbet/llvm/llvm/tools/clang/test/SemaCXX/static-assert-cxx17.cpp Line 76: static_assert failed due to requirement 'typename ExampleTypes::T(0)'

I must admit that I did not think of handling these, and now I think it would actually be better to tap into the pretty printer to avoid code duplication.

[courbet]

Handle remaining static_assert categories inside the prettyprinter.

[courbet]

clang-format patch

[courbet]

revert now unneeded changes.

[aaron.ballman]

include/clang/AST/Stmt.h

847 ↗

(On Diff #178012)

I'm pretty wary of long lists of parameters that include booleans with default values. Why is PrintCanonicalTypes not part of the printing policy? I think that's where we should keep all the knobs relating to how the printing happens.

[courbet]

move PrintQualifiedTypes to PrintingPolicy

include/clang/AST/Stmt.h

847 ↗

(On Diff #178012)

Good point, done.

[aaron.ballman]

lib/AST/StmtPrinter.cpp

78 ↗

(On Diff #178882)

Spurious formatting changes -- can revert this file.

lib/AST/TypePrinter.cpp

165 ↗

(On Diff #178882)

t doesn't meet the usual naming requirements; how about QT?

lib/Sema/SemaTemplate.cpp

3062 ↗

(On Diff #178882)

Why are you dropping the explicit here?

3124 ↗

(On Diff #178882)

Don't use auto here, please.

[courbet]

lib/AST/TypePrinter.cpp

165 ↗

(On Diff #178882)

I was keeping it for consistency with TypePrinter::print(QualType t,, but done.

lib/Sema/SemaTemplate.cpp

3062 ↗

(On Diff #178882)

Sorry, this is a revert of an approach where it had two parameters.

[courbet]

address review comments

[courbet]

clang-format diff

[aaron.ballman]

LGTM, but you should wait a few days before committing in case @Quuxplusone has comments.

[Quuxplusone]

LGTM. Tiny style suggestions, which I won't mind if you ignore.

include/clang/AST/Type.h

995 ↗

(On Diff #178888)

Nit: could this function have been left in-line, and just changed split() to splitAccordingToPolicy(this, Policy)?
Even simpler, could splitAccordingToPolicy be made a member function of QualType, so that most of these diffs could be simply s/split()/splitAccordingToPolicy(Policy)/ without introducing any new temporary variables or anything? I.e.

void getAsStringInternal(std::string &Str,
                          const PrintingPolicy &Policy) const {
    return getAsStringInternal(splitAccordingToPolicy(Policy), Str, Policy);
}

But if that would make the code harder to read instead of easier, then don't mind me.

998 ↗

(On Diff #178888)

Nit: this function's body hasn't changed, so personally I would have left it alone (not out-of-lined it).

[courbet]

address review comments

[courbet]

Thanks for the comments !

include/clang/AST/Type.h

995 ↗

(On Diff #178888)

I'd rather avoid polluting the QualType API with splitAccordingToPolicy() which is really only useful for this use case.

without introducing any new temporary variables or anything? i.e.

Actually this is independent of the above change, done.