[PATCH] D43248: [Attr] Fix parameter indexing for attributes

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.


================
Comment at: include/clang/AST/Attr.h:210-212
+  unsigned Idx;
+  bool HasThis;
+  bool IsValid;
----------------
aaron.ballman wrote:
> I think it might be best to mash these together using bit-fields:
> ```
> unsigned Idx : 30;
> unsigned HasThis : 1;
> unsigned IsValid : 1;
> ```
Good point.  Thanks.


================
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;
+  }
----------------
aaron.ballman wrote:
> 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.
Oops.  Thanks.


================
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); }
----------------
aaron.ballman wrote:
> `std::iterator` was deprecated in C++17, so you should manually expose these fields.
Now that sizeof(ParamIdx) == sizeof(unsigned), I can no longer find a reason to avoid ParamIdx arrays, and so I can no longer find a good justification for the iterator classes.


https://reviews.llvm.org/D43248