add getLastPtr and getLastOrNullPtr functions to ArrayLists

[slooWne]

as getLast exists, it would be useful to have a version that also returns a pointer to the last element.

[RetroDev256]

I'm not sure that this is a good idea. Pointers to ArrayList elements can be invalidated whenever the list needs to grow in capacity. I feel that the introduction of these functions will introduce bugs that are hard to track down.

[slooWne]

That's true,
However, some functions already act like this, for example addOne returns a pointeur to the last element, which can also become invalid.
I think these functions are useful in the same way as |*captures| and, precisely in a program where the last item changes often, always having to rewrite .items[arraylist_name.items.len - 1] is annoying, so .getLastPtr() would be more practical.

[RetroDev256]

I appreciate the response, yet I think the context in which these functions are used may be different.

I have used addOne immediately preceding the initialization of a value. It is handy for that specific purpose. On the other hand, I don't think that people who get a pointer to the last (already allocated) element of an ArrayList would immediately write to it in the same way as addOne. If not, then the invalidation bug remains. However if this were the use of such an operation, it may be more helpful to add a setLastAssumeCapacity (or similar) function, wrapped in an if (list.items.len != 0) { ... } block to handle an empty list. At that point, I don't think that the added function would be justified, as it is (in my opinion) more clear for the code to simply use list.items[list.items.len - 1], even though it's "annoying."

[slooWne]

The case I had in mind when adding these functions was for ArrayLists of structs that have a function modifying themselves:
list.getLastPtr().changeSomething()
But you're right, a setLast function could also be useful and perhaps more safe, but wouldn't allow what I mentioned above.
I don't think the [.len - 1] syntax adds that much safety - while being less readable - because the pointer is created anyway

[mrjbq7]

I think it's not a needed change, but perhaps one question would be to see what code is improved by this change.

For example, I believe this diff is equivalent using getLastPtr():

diff --git a/tools/incr-check.zig b/tools/incr-check.zig
index 08b8a21e3b..4c47a786ee 100644
--- a/tools/incr-check.zig
+++ b/tools/incr-check.zig
@@ -729,7 +729,7 @@ const Case = struct {
                     });
                 } else if (std.mem.eql(u8, key, "update")) {
                     if (updates.items.len > 0) {
-                        const last_update = &updates.items[updates.items.len - 1];
+                        const last_update = updates.getLastPtr();
                         last_update.changes = try changes.toOwnedSlice(arena);
                         last_update.deletes = try deletes.toOwnedSlice(arena);
                     }
@@ -769,7 +769,7 @@ const Case = struct {
                     try deletes.append(arena, val);
                 } else if (std.mem.eql(u8, key, "expect_stdout")) {
                     if (updates.items.len == 0) fatal("line {d}: expect directive before update", .{line_n});
-                    const last_update = &updates.items[updates.items.len - 1];
+                    const last_update = updates.getLastPtr();
                     if (last_update.outcome != .unknown) fatal("line {d}: conflicting expect directive", .{line_n});
                     last_update.outcome = .{
                         .stdout = std.zig.string_literal.parseAlloc(arena, val) catch |err| {
@@ -778,7 +778,7 @@ const Case = struct {
                     };
                 } else if (std.mem.eql(u8, key, "expect_error")) {
                     if (updates.items.len == 0) fatal("line {d}: expect directive before update", .{line_n});
-                    const last_update = &updates.items[updates.items.len - 1];
+                    const last_update = updates.getLastPtr();
                     if (last_update.outcome != .unknown) fatal("line {d}: conflicting expect directive", .{line_n});

                     var errors: std.ArrayListUnmanaged(ExpectedError) = .empty;
@@ -831,7 +831,7 @@ const Case = struct {
         }

         if (changes.items.len > 0) {
-            const last_update = &updates.items[updates.items.len - 1];
+            const last_update = updates.getLastPtr();
             last_update.changes = changes.items; // arena so no need for toOwnedSlice
             last_update.deletes = deletes.items;
         }

Here are some other possibilities, but i didn't verify them all.

$ rg "items.len - 1" | grep " &"
src/Compilation.zig:                            const top = &stack.items[stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:            const branch = &bt.function.branch_stack.items[bt.function.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const table = &self.branch_stack.items[self.branch_stack.items.len - 1].inst_table;
src/arch/sparc64/CodeGen.zig:        const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:        const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/sparc64/CodeGen.zig:    const branch = &self.branch_stack.items[self.branch_stack.items.len - 1];
src/arch/riscv64/CodeGen.zig:    const table = &func.branch_stack.items[func.branch_stack.items.len - 1].inst_table;
test/nvptx.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
src/arch/wasm/CodeGen.zig:    return &cg.branches.items[cg.branches.items.len - 1];
tools/incr-check.zig:                        const last_update = &updates.items[updates.items.len - 1];
tools/incr-check.zig:                    const last_update = &updates.items[updates.items.len - 1];
tools/incr-check.zig:                    const last_update = &updates.items[updates.items.len - 1];
tools/incr-check.zig:            const last_update = &updates.items[updates.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
test/src/Cases.zig:    return &ctx.cases.items[ctx.cases.items.len - 1];
lib/docs/wasm/markdown/Parser.zig:            .list => &p.pending_blocks.items[p.pending_blocks.items.len - 1],
lib/std/debug/Dwarf/expression.zig:                    mem.swap(Value, &self.stack.items[self.stack.items.len - 1], &self.stack.items[self.stack.items.len - 2]);
lib/std/debug/Dwarf.zig:    return &di.abbrev_table_list.items[di.abbrev_table_list.items.len - 1];
lib/std/fs/Dir.zig:            var top = &self.stack.items[self.stack.items.len - 1];
lib/std/fs/Dir.zig:                        top = &self.stack.items[self.stack.items.len - 1];
lib/std/fs/Dir.zig:        var top = &stack.items[stack.items.len - 1];
lib/std/zig/llvm/BitcodeReader.zig:    const state = &bc.stack.items[bc.stack.items.len - 1];
lib/std/Build/Step/CheckObject.zig:    const last = &check_object.checks.items[check_object.checks.items.len - 1];
lib/std/Build/Step/CheckObject.zig:    const last = &check_object.checks.items[check_object.checks.items.len - 1];
lib/std/Build/Step/CheckObject.zig:    const last = &check_object.checks.items[check_object.checks.items.len - 1];
lib/std/Build/Step/CheckObject.zig:    const last = &check_object.checks.items[check_object.checks.items.len - 1];
lib/std/array_list.zig:            return &self.items[self.items.len - 1];
lib/std/array_list.zig:            return &self.items[self.items.len - 1];
lib/std/json/dynamic.zig:                var object = &stack.items[stack.items.len - 1].object;
lib/fuzzer.zig:        return &l.items[l.items.len - 1];

It's not that many places and the code looks fine to me as-is, but one compelling reason might be to reduce the risk of typos in miscalculating the last item index...

[RetroDev256]

Replying to slooWne:

Interesting case - I had not considered the argument of a structure which requires a pointer to itself. In fact, how often does this ever occur? I don't remember a time I've ever needed to do that. As for the list.items[list.items.len - 1] syntax I mentioned, I didn't mean to communicate this:

const last_element = &list.items[list.items.len - 1];
// ...
last_element.someOperation();

I intended to communicate this:

list.items[list.items.len - 1].someOperation();

Or even this:

const last_index = list.items.len - 1;
list.items[last_index].someOperation();

As for the comment made by mrjbq7:

I don't believe that the change would impact code generation, especially for an optimizing compiler. The change is trivial enough that either way is likely identical, though I'd love to be proven wrong. I don't think that miscalculating the last index (via a typo) is significant enough to warrant a special purposed function though.

In the end we are discussing nuances about a function that makes code slightly more concise. Is Zig the type of language where we want to abstract single (& simple) lines of code behind a function call? I don't think so. I argue that the information conveyed in either case is the same (and may be more clear if written explicitly).

EDIT: I read your comment wrong mrjbq7 - I see now you were talking about the code, and not code generation. In that case, I am in agreement.

[RyanDeivert555]

I think before thinking about adding different variations of the getLast method, it is important to know the rational on why getLast exists in the first place. It seems very out of place, given there is no get(self: Self, index: usize) ?T sort of method

[slooWne]

I guess it's like pop, it's for stack like usage of arrayLists