Page MenuHomePhabricator

Sema: Let InitListExpr have dependent type instead of 'void'
Needs ReviewPublic

Authored by aaronpuchert on Nov 13 2021, 4:18 PM.

Details

Summary

It was always just a placeholder, but not a particularly good one: the
expression certainly has a value in most cases, and 'void' can also not
serve as an indicator for an undeduced type because of void{}.

Instead I think the dependent type is a good fit: an initializer list
is an expression that can instantiate to expressions of arbitrary types:
One could say that {...} is a map

T : Type → T, T ↦ T{...}

The type parameter can be explicitly specified or be deduced from an
implicit conversion target type. So one could for example view {1} as

struct InitList1 {
  template<typename T>
  operator T() { return T{1}; }
};

That in itself isn't type-dependent, but note that we let InitListExprs
have the actual type they initialize in the end, so in this case 'T'
instead of 'InitList1'.

A test failure in CXX/expr/expr.prim/expr.prim.lambda/p4.cpp revealed
that we were emitting errors "return type ... must match previous
return type ... when lambda expression has unspecified explicit return
type" even after diagnosing InitListExprs with "cannot deduce lambda
return type from initializer list", which in this case didn't show up
because it accidentally matched with the actual 'void' of the other
return statements.

Diff Detail

Unit TestsFailed

TimeTest
240 msx64 debian > Clang Tools.clang-tidy/checkers::cppcoreguidelines-owning-memory.cpp
Script: -- : 'RUN: at line 1'; /usr/bin/python3.9 /var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/test/../test/clang-tidy/check_clang_tidy.py /var/lib/buildkite-agent/builds/llvm-project/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp cppcoreguidelines-owning-memory /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/Output/cppcoreguidelines-owning-memory.cpp.tmp

Event Timeline

aaronpuchert requested review of this revision.Nov 13 2021, 4:18 PM
aaronpuchert created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2021, 4:18 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Adapt clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp and run clang-format.

Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2021, 6:17 PM

We could also introduce a separate (placeholder) type for initializer lists and perhaps also ParenListExpr if the dependent type seems misleading to you.

CC @JonasToth for the changes to clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp
306 ↗(On Diff #387054)

If the warning is looking at this in the original template, we're initializing a gsl::owner<T *> which is type-dependent. So we can't carry out the initialization yet.

Got the impression earlier that the lifetime checks want to “understand” templates which can of course not work because templates can be specialized, overload resolution is only performed on the instantiation, and so on.

clang/test/SemaOpenCLCXX/address-space-references.clcpp
25

Somehow we seem to fail matching the initializer list, but not report that, only on type mismatch later. I think we should either go through with the initialization, then have a short __attribute__((ext_vector_type(2))) and complain that we can't bind the reference to that, or complain when processing the initializer list.

But that's unrelated to this change, void was also not correct.

FWIW, I think this direction seems reasonable to me. Before semantic analysis, we can't know the type, it depends on the context that we haven't analyzed yet. However, I'd ultimately like to hear from @rsmith just in case I am missing some concerns with the changes.