Page MenuHomePhabricator

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

Authored by courbet on Dec 11 2018, 4:06 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Dec 11 2018, 4:06 AM
courbet updated this revision to Diff 177681.Dec 11 2018, 4:09 AM

cosmetics

Quuxplusone added inline comments.Dec 11 2018, 7:05 AM
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 planned changes to this revision.Dec 12 2018, 9:11 AM
courbet marked an inline comment as done.
courbet added inline comments.
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 updated this revision to Diff 178010.Dec 13 2018, 12:41 AM

Handle remaining static_assert categories inside the prettyprinter.

courbet updated this revision to Diff 178011.Dec 13 2018, 12:44 AM

clang-format patch

courbet updated this revision to Diff 178012.Dec 13 2018, 12:46 AM

revert now unneeded changes.

aaron.ballman added inline comments.Dec 14 2018, 5:35 AM
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 updated this revision to Diff 178882.Dec 19 2018, 6:50 AM
courbet marked 2 inline comments as done.

move PrintQualifiedTypes to PrintingPolicy

include/clang/AST/Stmt.h
847 ↗(On Diff #178012)

Good point, done.

aaron.ballman added inline comments.Dec 19 2018, 7:15 AM
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 marked 6 inline comments as done.Dec 19 2018, 7:44 AM
courbet added inline comments.
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 updated this revision to Diff 178887.Dec 19 2018, 7:44 AM
courbet marked 2 inline comments as done.

address review comments

courbet updated this revision to Diff 178888.Dec 19 2018, 7:45 AM

clang-format diff

aaron.ballman accepted this revision.Dec 19 2018, 8:36 AM

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

This revision is now accepted and ready to land.Dec 19 2018, 8:36 AM

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 updated this revision to Diff 179018.Dec 19 2018, 11:49 PM
courbet marked 3 inline comments as done.

address review comments

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.

This revision was automatically updated to reflect the committed changes.