aaron.ballman added a comment. In https://reviews.llvm.org/D27424#636496, @george.burgess.iv wrote:
> Do we have a page that details when we should/shouldn't use `auto`? I was > under the impression that it was preferred only in cases where the type's > spelled out (e.g. `cast<Foo>`, ...). (To be clear, I'm happy to use it in > loops, too; I'd just like to know if we have guidelines about it. :) ) We do: http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable Btw, there's a question in there about late parsing that Phab won't let me delete, feel free to ignore it. ;-) ================ Comment at: include/clang/Basic/Attr.td:1584 +def DiagnoseIf : InheritableAttr { + let Spellings = [GNU<"diagnose_if">]; + let Subjects = SubjectList<[Function]>; ---------------- george.burgess.iv wrote: > aaron.ballman wrote: > > I think this should also have a C++ spelling under the clang namespace. > Agreed. Though, it looks like that would requires us teaching the parser how > to late-parse c++11 attrs if we wanted to do it properly. Given the size of > this patch, I think that can be done as a follow-up. That seems reasonable to me. ================ Comment at: include/clang/Sema/Sema.h:2616 + DiagnoseIfAttr * + checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef<Expr *> Args, + SmallVectorImpl<DiagnoseIfAttr *> &Nonfatal, ---------------- george.burgess.iv wrote: > aaron.ballman wrote: > > Can this (and the other new functions) accept the `FunctionDecl` as a > > `const FunctionDecl *` instead? > Not easily, unless you'd prefer to see `const` here and a `const_cast` or two > in `checkArgDependentDiagnoseIf` over what we have now. I have no preference, > so if you would prefer to see that, I'm happy to do it. > > If `ExprResult` and initialization routines like > `Sema::PerformCopyInitialization` were happy to take a `const Expr *`, this > would be a different story :) That sounds like more churn than it's worth, to me. It'd be nice if we were better about const-correctness, but that's not this patch's problem to solve. ;-) ================ Comment at: lib/AST/ExprConstant.cpp:10371 + assert(MD && "Don't provide `this` for non-methods."); + assert(!MD->isStatic() && "Don't provide `this` for non-static methods."); +#endif ---------------- I think the message means to say "Don't provide 'this' for static methods." ================ Comment at: lib/Parse/ParseDecl.cpp:371-372 return; } // These may refer to the function arguments, but need to be parsed early to ---------------- In the last patch, this code was checking for `enable_if` or `diagnose_if`, but now only checks for `enable_if`. Do you not need this for `diagnose_if` because it's now late parsed? ================ Comment at: lib/Sema/SemaDeclAttr.cpp:949 +#ifndef NDEBUG + if (auto *MD = dyn_cast<CXXMethodDecl>(FD)) + ClassType = MD->getParent(); ---------------- `const auto *` https://reviews.llvm.org/D27424 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
