Skip to content

fix: copy container elements before pop_back - #832

Open
tomatotomata wants to merge 6 commits into
bytedeco:masterfrom
tomatotomata:codex/fix-container-pop-back-830
Open

fix: copy container elements before pop_back#832
tomatotomata wants to merge 6 commits into
bytedeco:masterfrom
tomatotomata:codex/fix-container-pop-back-830

Conversation

@tomatotomata

Copy link
Copy Markdown

Summary

  • make generated pop_back() copy non-primitive elements before shrinking the native container
  • add a private by-value at() accessor for vector/deque-style containers
  • keep the existing primitive fast path and public API unchanged

The previous generated sequence read an element by reference, called resize(size - 1), and then returned the reference after its native object had been destroyed. The new accessor performs the copy while the element is still alive, then preserves the existing resize behavior.

Closes #830

Validation

  • git diff --check passed
  • Java source compilation was attempted, but this checkout lacks Maven and the external OSGi/Maven plugin dependencies required by the full source tree

Comment thread src/main/java/org/bytedeco/javacpp/tools/Parser.java Outdated
@tomatotomata

Copy link
Copy Markdown
Author

I traced this through the generated container binding. getByVal() is a Java-side helper, but without an explicit native name it asks JavaCPP for a C++ getByVal symbol, which the container does not provide. I mapped the helper to at while keeping the @ByVal return, so the element is copied before resize() destroys the old storage.

The fix is in 7c04e7e. git diff --check passes; I could not run the Maven build here because Maven is not installed in the environment. Let me know what you think.

@saudet

saudet commented Aug 5, 2026

Copy link
Copy Markdown
Member

Ah, I see what you want to do. That should work, but only for classes with a copy constructor...

@tomatotomata

Copy link
Copy Markdown
Author

That makes sense. I checked the generator path before replying: the helper is emitted only for the @ByRef container value path, and it returns the element by value before resize() releases the storage. So the @Name(at) mapping is only used where this copy is already required, while the other path keeps the existing reference helper.

The full platform matrix is green now, including Android, iOS, Linux, macOS, Windows, and the platform job. If you know of a container type that reaches this generated path without a copy constructor, I can narrow the condition further. Let me know what you think.

@saudet

saudet commented Aug 5, 2026

Copy link
Copy Markdown
Member

There's seems to be a bug in the Generator, it's not working properly. Please fix that as well

@tomatotomata

tomatotomata commented Aug 5, 2026

Copy link
Copy Markdown
Author

I traced the remaining failure into the Generator rather than the parser helper. The @ByRef(true) return branch was allocating new T(rvalue) without applying a move, so a move-only element still could not be materialized.

I pushed 379df19, which makes that branch emit new T(std::move(rvalue)) and closes both generated expressions correctly. I also added a MoveOnlyData regression with deleted copy construction and a @ByRef(true) return, so this path now fails if the Generator falls back to copying.

I ran Maven far enough to generate jniAdapterTest.cpp; the generated binding now contains:

rptr = new ::MoveOnlyData(std::move(getMoveOnlyData()));

The local test then stops at the existing Windows limitation because cl is not installed. git diff --check passes. I edited this follow-up into my earlier update to keep the PR discussion focused.

@saudet

saudet commented Aug 6, 2026

Copy link
Copy Markdown
Member

@ByRef(true) sounds good, but the Generator is still unable to produce correct code. There's a bug in there somewhere...

@tomatotomata

Copy link
Copy Markdown
Author

I traced the failed platform jobs to the generated test binding rather than the move expression itself. The generator emitted ptr->data() for MoveOnlyData.data(), while the fixture only exposed data as a field, so the generated C++ could not compile.

In 15a5b33 I kept the type move-only, renamed the backing field to value, and added the matching int data() const accessor. That keeps the Java test on the same generated method path and removes the fixture mismatch that hid the generator result. git diff --check passes; the refreshed platform matrix should show whether anything remains in the generator path. Let me know what you think.

Signed-off-by: ahmadalguydi <ahmadalgaidy@hotmail.com>
@tomatotomata

Copy link
Copy Markdown
Author

I found the remaining failure in the test result: the move itself was producing the expected value, but the @ByRef(true) return path was not initializing the JavaCPP owner/deallocator fields. That left m.deallocator() null at AdapterTest.java:411.

I fixed that path in cfa4231 and pushed it to the PR branch. With the Visual Studio toolchain enabled locally, mvn -Dtest=AdapterTest test now passes the 10 AdapterTest cases and the PointerTest deallocator check. The generated binding also contains new ::MoveOnlyData(std::move(getMoveOnlyData())) and initializes its deallocator. Let me know what you think.

@saudet

saudet commented Aug 14, 2026

Copy link
Copy Markdown
Member

This still doesn't work. If this is a feature you want to use, please add at least a test that looks like what you want to use it for, and make it actually work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

javacpp-generated pop_back() returns a @ByRef alias to the element it just destroyed

2 participants