bolshakov-a added a comment.
Btw, formatting of unrelated lines has leaked into `TemplateBase.h`. Sorry.
================
Comment at: clang/lib/AST/MicrosoftMangle.cpp:1624-1626
+ return const_cast<ValueDecl *>(
+ V.getLValueBase().dyn_cast<const ValueDecl *>());
+}
----------------
aaron.ballman wrote:
> Does this work or is the `const_cast` actually required?
No, it doesn't compile, likewise the standard C++ `dynamic_cast` cannot remove
constness.
================
Comment at: clang/lib/AST/TemplateBase.cpp:408-409
case Integral:
- getAsIntegral().Profile(ID);
getIntegralType().Profile(ID);
+ getAsIntegral().Profile(ID);
+ break;
----------------
aaron.ballman wrote:
> Why did the order of these calls change?
I don't know, it is from 9e08e51a20d0d2. I've tried to invert the order along
with the order for `StructuralValue`, and all tests have been passed.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:5983-5985
+ Expr *E = Result.get();
+ if (!isa<ConstantExpr>(E))
+ E = ConstantExpr::Create(S.Context, Result.get(), Value);
----------------
aaron.ballman wrote:
> I thought we could run into situations where we had a `ConstantExpr` but it
> did not yet have its result stored in it. Should we assert that isn't the
> case here?
If I understand correctly, the sole place where `ConstantExpr` is constructed
which may occur here is `BuildExpressionFromNonTypeTemplateArgumentValue`
function, and a value is set into it there. Should I add the assertion into
code?
================
Comment at: clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp:56
+using CF = ComplexFloat<1.0f + 3.0fi>;
+using CF = ComplexFloat<3.0fi + 1.0f>;
----------------
aaron.ballman wrote:
> bolshakov-a wrote:
> > shafik wrote:
> > > Can we add an enum example e.g.:
> > >
> > > ```
> > > enum E{ E1, E2};
> > > template <E> struct SE {};
> > > using IPE = SE<E1>;
> > > using IPE = SE<E2>;
> > >
> > > ```
> > What for? Enumerators as non-type template arguments are allowed since
> > C++98, AFAIK. And this test is about changes in C++20.
> Sometimes we're lacking coverage for existing features, so when updating code
> in the area, we'll sometimes ask for extra coverage just to be sure we're not
> regressing something we think might not have a lot of existing test coverage.
`temp_arg_nontype.cpp` test already has some `enum` cases. If a case with type
alias should be added, it shoud be added there, not in the
`temp_arg_nontype_cxx20.cpp`, I think.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140996/new/
https://reviews.llvm.org/D140996
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
