hokein added inline comments.
================
Comment at: clang/lib/AST/ComputeDependence.cpp:491
+ auto D =
+ toExprDependence(E->getType()->getDependence()) | ExprDependence::Error;
for (auto *S : E->subExpressions())
----------------
sammccall wrote:
> Dropping type-dependence seems like the point here and is risky, doesn't
> dropping value- and instantiation-dependence introduce unneccesary extra risk?
>
> Probably correct and maybe useful, but splitting it into a separate change
> and maybe delaying it might make it easier to get the principal change to
> stick.
> Dropping type-dependence seems like the point here and is risky, doesn't
> dropping value- and instantiation-dependence introduce unneccesary extra risk?
maybe, but you are right, we should be conservative.
separating this patch seems good to me:
1) create a new patch like what we do here,
2) this patch just getting rid of the type bit (still set the
value/instantiation bits)
with 1, we will get rid of the value/instantiation-depend bits (for the default
`Ctx.DependentType` case), and we get closer to our goal.
================
Comment at: clang/lib/AST/Expr.cpp:4656
#ifndef NDEBUG
+ assert(!T.isNull());
for (auto *E : SubExprs)
----------------
sammccall wrote:
> Why is this in ifndef?
> Actually, why is the loop in ifndef? surely the compiler can inline and
> remove an empty loop over an arrayref?
removed.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:16427
QualType EltTy = Context.getBaseElementType(T);
- if (!EltTy->isDependentType()) {
+ if (!EltTy->isDependentType() && !EltTy->containsErrors()) {
if (RequireCompleteSizedType(Loc, EltTy,
----------------
sammccall wrote:
> why is this needed/does it help?
> I would have thought in the cases where we now don't consider the type
> dependent, the *type* wouldn't contain errors (rather the expression owning
> the type would).
>
> Is this just for decltype?
yeah, decltype is the only case we have been discovered so far.
================
Comment at: clang/test/CodeCompletion/member-access.cpp:276
+struct S { int member; };
+S overloaded(int);
----------------
sammccall wrote:
> I think there's an interesting test of the "viable" case where you have a
> const/non-const overload set with different return types:
> ```
> class Collection {
> const char *find(int) const;
> char* find(int) const;
> };
> void test1(const Collection &C, Collection &NC) {
> C.find(); // missing arg, type const char*
> NC.find(); // missing arg, is type NC or is it unresolved?
> }
> ```
>
>
> (Not sure if it's best written as a codecompletion test, I guess AST dump is
> better)
oh, this is a good point, this patch doesn't capture the type for member
functions, it is in a different code path (probably we should handle that in
`BuildCallToMemberFunction`).
I'd prefer to handle it in a separate patch.
================
Comment at: clang/test/SemaCXX/enable_if.cpp:417
-constexpr int B = 10 + // the carat for the error should be pointing to the
problematic call (on the next line), not here.
- callTemplated<0>(); // expected-error{{initialized by a constant
expression}} expected-error@-3{{no matching function for call to 'templated'}}
expected-note{{in instantiation of function template}}
expected-note@-10{{candidate disabled}}
+constexpr int B = 10 + // expected-error {{initialized by a constant
expression}}
+ callTemplated<0>(); // expected-error@-3{{no matching function for call to
'templated'}} expected-note{{in instantiation of function template}}
expected-note@-10{{candidate disabled}} expected-note {{in call to
'callTemplated()'}} expected-note@-3 {{subexpression not valid in a constant
expression}}
----------------
sammccall wrote:
> hokein wrote:
> > this diagnostic is changed slightly (without `-frecovery-ast-type`). I
> > think this is acceptable.
> >
> > before this patch:
> >
> > ```
> > /tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
> > return templated<N>(); // expected-error {{no matching function for call
> > to 'templated'}}
> > ^~~~~~~~~~~~
> > /tmp/t5.cpp:13:5: note: in instantiation of function template
> > specialization 'enable_if_diags::callTemplated<0>' requested here
> > callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}}
> > expected-note@-6 {{subexpression not valid in a constant expression}}
> > ^
> > /tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
> > template <int N> constexpr int templated() __attribute__((enable_if(N,
> > ""))) {
> > ^ ~
> > /tmp/t5.cpp:13:5: error: constexpr variable 'B' must be initialized by a
> > constant expression
> > callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}}
> > expected-note@-6 {{subexpression not valid in a constant expression}}
> > ^~~~~~~~~~~~~~~~~~
> > 2 errors generated.
> > ```
> >
> > vs after this patch:
> >
> > ```
> > /tmp/t5.cpp:7:10: error: no matching function for call to 'templated'
> > return templated<N>(); // expected-error {{no matching function for call
> > to 'templated'}}
> > ^~~~~~~~~~~~
> > /tmp/t5.cpp:13:5: note: in instantiation of function template
> > specialization 'enable_if_diags::callTemplated<0>' requested here
> > callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}}
> > expected-note@-6 {{subexpression not valid in a constant expression}}
> > ^
> > /tmp/t5.cpp:2:32: note: candidate disabled: <no message provided>
> > template <int N> constexpr int templated() __attribute__((enable_if(N,
> > ""))) {
> > ^ ~
> > /tmp/t5.cpp:12:15: error: constexpr variable 'B' must be initialized by a
> > constant expression
> > constexpr int B = 10 + // expected-error {{constexpr variable 'B' must be
> > initialized by a constant expression}}
> > ^
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > /tmp/t5.cpp:7:10: note: subexpression not valid in a constant expression
> > return templated<N>(); // expected-error {{no matching function for call
> > to 'templated'}}
> > ^
> > /tmp/t5.cpp:13:5: note: in call to 'callTemplated()'
> > callTemplated<0>(); // expected-note {{in call to 'callTemplated()'}}
> > expected-note@-6 {{subexpression not valid in a constant expression}}
> > ^
> > 2 errors generated.
> > ```
> >
> >
> Yeah, this is noisier but seems OK.
> Can you take a look at the blame for the split-line test and see if there was
> any special code to support it that can be cleaned up?
`git blame` showed
https://github.com/llvm/llvm-project/commit/c7d3e609fbf05d1a1236f99efd1e2fd344554f4b,
but I didn't see any special code that we need clean up.
================
Comment at: clang/test/SemaCXX/recovery-expr-type.cpp:12
+ // no crash.
+ int s = sizeof(make_indestructible()); // expected-error {{deleted}}
+}
----------------
sammccall wrote:
> what if we make this constexpr and try to static-assert on it, use it as the
> size of an array etc?
added tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79160/new/
https://reviews.llvm.org/D79160
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
