[PATCH] D93654: [TableGen] Change getAllDerivedDefinitions() to return an ArrayRef

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D93654#2468298 <https://reviews.llvm.org/D93654#2468298>, @lattner wrote:

> Yeah C++ syntax isn't great.  If you take the path of returning arrayref, it is a bad idea to allow it to convert the a 'const&'.  My understanding is that operator you found on arrayref will make a copy of the data (defeating the whole purpose of this patch :-), and that the lifetime will be bound to the statement.  This means the reference will actually dangle.  :-(

In this case I believe you'd get Reference Lifetime Extension <https://abseil.io/tips/107> here and the code would be safe/correct, but generally not a good idea to rely on that syntax anyway, except in some very niche template/generic code situations.

> You can either keep it as a const& return or return an array ref and change the callers to use 'auto' or an explicit ArrayRef declaration.

Yeah, for the best - I'd probably use ArrayRef personally, since 'auto' hides whether the type is lightweight or heavyweight & at least I'd tend to read as heavyweight unless there were other context clues. (I guess we're seeing more iterator_ranges and other value typed lightweight types that we're probably using auto on, so maybe my reading will/has already changed) Probably short enough to write out without harming readability greatly.

In D93654#2468467 <https://reviews.llvm.org/D93654#2468467>, @Paul-C-Anagnostopoulos wrote:

> The modification occurs on line 65:
>
> llvm::erase_if(defs, ...
>
> My temptation would be to add a macro to make it clear what's going on:
>
>   MODIFY_DEF_VECTOR std::vector<llvm::Record *> defs =
>       recordKeeper.getAllDerivedDefinitions("OpInterface");
>
> But I haven't seen that sort of thing done in LLVM.

Yeah, as @lattner said, a macro isn't suitable here (you may find such macros for things like "mark this variable as used" because the macro would expand to an attribute the compiler can use for enforcement or warning suppression, etc - but if the macro never expands to anything it's generally better to use a comment instead (because the comment can use real prose, because it doesn't imply something semantically meaningful that isn't, etc)). If you want to make the conversion explicit you can call the 'vec()' function - I think that'd be enough for me, without further comment, to make it clear the author intended the conversion. But comment probably doesn't hurt either.


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

https://reviews.llvm.org/D93654