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

[Aaron Ballman via Phabricator via cfe-commits]

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