[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked 13 inline comments as done.
riccibruno added a comment.

In D57098#1367769 <https://reviews.llvm.org/D57098#1367769>, @aaron.ballman wrote:

> This is looking great! I've got some comments, mostly nits. Just to double-check, have you verified that these changes do not break any existing tests (in clang or clang-tools-extra)?


I ran the usual assert build/asan+assert build/msan+assert build.
To be clear I know that there are some changes required but I just wanted to put it out here to get
some feedback on the basic idea (which is to use some kind of proxy iterator).

Some added comments inline.



================
Comment at: include/clang/AST/Expr.h:5026
+  /// result expression in the case where the generic selection expression
+  /// is not result-dependent. The result index is equal to -1u if and only
+  /// if the generic selection expression is result-dependent.
----------------
aaron.ballman wrote:
> `~0U` instead of `-1u` so it doesn't suggest a weird type mismatch?
Yes indeed (or `std::numeric_limits<unsigned>::max()` as suggested below).


================
Comment at: include/clang/AST/Expr.h:5043
+  unsigned numTrailingObjects(OverloadToken<Stmt *>) const {
+    return 1 + getNumAssocs();
+  }
----------------
aaron.ballman wrote:
> I think it would be good to put a comment here like "Add one to account for the controlling expression; the remainder are the associated expressions."
Will do.


================
Comment at: include/clang/AST/Expr.h:5094
+  public:
+    using iterator_category = std::forward_iterator_tag;
+    using value_type = AssociationTy<Const>;
----------------
aaron.ballman wrote:
> It seems like this should be pretty trivial to make into a random access iterator, or am I missing something?
Indeed, at the cost of some boilerplate. I can totally do this but from what I understood the motivation was to use this in ranges, and for this a forward iterator is good enough (although see the next comment)


================
Comment at: include/clang/AST/Expr.h:5097
+    using difference_type = std::ptrdiff_t;
+    using pointer = AssociationTy<Const>;
+    using reference = AssociationTy<Const>;
----------------
aaron.ballman wrote:
> Cute, but I suspect this may come back to bite us at some point. For instance, if someone thinks they're working with a real pointer, they're likely to expect pointer arithmetic to work when it won't (at least they'll get compile errors though).
Hmm, but `pointer` is just the return type of `operator->` no ? Is it actually required to behave like a pointer ? The only requirement I can find is that `It->x` must be equivalent to `(*It).x`, which is true here.

Also looking at the requirements for forward iterators I think that this iterator should actually be downgraded to an input iterator, because of the requirement that `reference = T&`.


================
Comment at: include/clang/AST/Expr.h:5101
+    AssociationTy<Const> operator*() const {
+      return AssociationTy<Const>(cast<Expr>(*E), *TSI,
+                                  Offset == SelectedOffset);
----------------
aaron.ballman wrote:
> This should return `reference` instead.
yes


================
Comment at: include/clang/AST/Expr.h:5104
+    }
+    AssociationTy<Const> operator->() const { return **this; }
+    AssociationIteratorTy &operator++() {
----------------
aaron.ballman wrote:
> This should return `pointer` instead.
yes


================
Comment at: include/clang/AST/Expr.h:5108
+      TSI += 1;
+      Offset += 1;
+      return *this;
----------------
yes


================
Comment at: include/clang/AST/Expr.h:5186
+  /// Whether this generic selection is result-dependent.
+  bool isResultDependent() const { return ResultIndex == -1U; }
 
----------------
aaron.ballman wrote:
> `std::numeric_limits<unsigned>::max()`?
yes


================
Comment at: include/clang/AST/Expr.h:5208
+  ArrayRef<Expr *> getAssocExprs() const {
+    return {reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>() +
+                                            ASSOC_EXPR_START),
----------------
aaron.ballman wrote:
> Do we need to use `reinterpret_cast` here, or can this be done through llvm's casting machinery instead?
I believe that it is needed, because `Stmt **` is not otherwise convertible to `Expr **`. This is all over the place in the statement/expression nodes, and indeed it depends on the layout of `Expr` and `Stmt`.


================
Comment at: include/clang/AST/Expr.h:5219
+  Association getAssociation(unsigned I) {
+    assert((I < getNumAssocs()) &&
+           "Out-of-range index in GenericSelectionExpr::getAssociation!");
----------------
aaron.ballman wrote:
> Spurious parens can be removed.
>From the precedence rules yes, but I thought that some gcc bots were
warning about this (or maybe I am mistaken and this is in an other situation) 


================
Comment at: lib/Sema/SemaPseudoObject.cpp:148
+
+        for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+          Expr *assoc = Assoc.getAssociationExpr();
----------------
aaron.ballman wrote:
> Probably have to go with `assoc` here.
Will do.


================
Comment at: lib/Sema/SemaPseudoObject.cpp:149
+        for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+          Expr *assoc = Assoc.getAssociationExpr();
+          if (Assoc.isSelected())
----------------
aaron.ballman wrote:
> This should be renamed regardless; probably `assocExpr`
Will do.


================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1040
+    TSIs[I] = GetTypeSourceInfo();
+  ;
 }
----------------
aaron.ballman wrote:
> Spurious semi-colon.
Oups.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57098/new/

https://reviews.llvm.org/D57098