aaron.ballman added a comment.
(Review is incomplete, but here are some initial comments.)
================
Comment at: include/clang/AST/Attr.h:210-212
+ unsigned Idx;
+ bool HasThis;
+ bool IsValid;
----------------
I think it might be best to mash these together using bit-fields:
```
unsigned Idx : 30;
unsigned HasThis : 1;
unsigned IsValid : 1;
```
================
Comment at: include/clang/AST/Attr.h:218
+ ParamIdx() : Idx(0), HasThis(false), IsValid(false) {}
+ /// \param Idx is the parameter index as it is normally specified in
+ /// attributes in the source: one-origin including any C++ implicit this
----------------
Add some newlines between the various declarations in the class -- everything
is condensed a bit too much without it.
================
Comment at: include/clang/AST/Attr.h:225
+ ParamIdx(unsigned Idx, const Decl *D) : Idx(Idx), IsValid(true) {
+ if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D))
+ HasThis = FD->isCXXInstanceMember();
----------------
`const auto *`
================
Comment at: include/clang/AST/Attr.h:227-228
+ HasThis = FD->isCXXInstanceMember();
+ else
+ HasThis = false;
+ }
----------------
Might as well move this into the member initializer.
================
Comment at: include/clang/AST/Attr.h:238-243
+ ParamIdx &operator=(const ParamIdx &I) {
+ Idx = I.Idx;
+ HasThis = I.HasThis;
+ IsValid = I.IsValid;
+ return *this;
+ }
----------------
Is this necessary? There should be an implicit copy assignment operator for
this class, and it's a bit strange to have a copy assignment but no copy
constructor.
================
Comment at: include/clang/AST/Attr.h:264
+ /// constructing new attributes from a source-like specification.
+ unsigned atSrc() const {
+ assert(isValid() && "ParamIdx must be valid");
----------------
I'd prefer more clear names, like "getSourceIndex()" and "getASTIndex()".
================
Comment at: include/clang/AST/Attr.h:282
+ /// This is the encoding primarily used in CodeGen.
+ unsigned atLLVM() const {
+ assert(isValid() && "ParamIdx must be valid");
----------------
Perhaps `getLLVMIndex()` for this one.
================
Comment at: include/clang/AST/Attr.h:291-292
+template <typename DerivedT, typename ValueT>
+class ParamIdxItrBase : public std::iterator<std::input_iterator_tag, ValueT,
+ ptrdiff_t, ValueT *, ValueT &> {
+ DerivedT &der() { return *static_cast<DerivedT *>(this); }
----------------
`std::iterator` was deprecated in C++17, so you should manually expose these
fields.
https://reviews.llvm.org/D43248
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
