aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Please add test cases for this new functionality.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7665
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx) {
+ assert(E);
+
----------------
You should add a message to your assertion in case it is triggered. e.g.,
`assert(E && "attempting to get a constexpr int out of a null Expr");` or
something along those lines.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7665
+static llvm::APSInt getConstexprInt(const Expr *E, const ASTContext& Ctx)
+{
+ assert(E);
----------------
aaron.ballman wrote:
> The curly brace should bind to the end of the function parameter list. I'd
> just run the whole patch through clang-format and then fix up anything that
> goes wrong in the .td files.
This is still a bit off -- the `&` should bind to `Ctx`.
================
Comment at: lib/CodeGen/TargetInfo.cpp:7669
+ llvm::APSInt Tmp{32, 0};
+ E->EvaluateAsInt(Tmp, Ctx);
+
----------------
aaron.ballman wrote:
> Are you expecting this to always succeed, or are you relying on the
> initialized value in the case where this fails?
This question still applies.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:5473
+static bool checkAllAreIntegral(Sema &S, const AttributeList &Attr) {
+ for (auto i = 0u; i != Attr.getNumArgs(); ++i) {
+ Expr* E = Attr.getArgAsExpr(i);
----------------
aaron.ballman wrote:
> Don't use `auto` here either, just use `unsigned`. Also `i` doesn't match our
> coding standard.
Only partially fixed; the naming convention is still not being followed.
https://reviews.llvm.org/D39857
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
