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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
