Skip to content

Conversation

fliiiix
Copy link
Contributor

@fliiiix fliiiix commented Aug 13, 2025

new_p is a local addr but is owned now by slice_
thus the life time does not end at the end of the function

The newest compilers had opinions about returning new_p here.

@github-actions github-actions bot added the c++ label Aug 13, 2025
@fliiiix
Copy link
Contributor Author

fliiiix commented Aug 13, 2025

@aardappel should be easy to merge, maybe there would be a way to rewrite this to be simpler for humans and compiler but it wasn't clear to me how

@aardappel
Copy link
Collaborator

It doesn't seem so great to be putting compiler specific warning toggles in the code like this.

What is the exact error? Can it be fixed in different ways, for example to construct the slice directly in slice_ such that there is no new_slice to warn about? Or to make the assignment between the two a move operation so it is clear to the compiler the local is not in use anymore? etc.

new_p is a local addr but is owned now by slice_
thus the life time does not end at the end of the function
@fliiiix fliiiix force-pushed the bugfix/ignore-wrong-warning branch from 7df8dd6 to 42132ee Compare August 18, 2025 13:29
@fliiiix
Copy link
Contributor Author

fliiiix commented Aug 18, 2025

The error/warning message with GCC 14.3 is:

src/vendor/flatbuffers/include/flatbuffers/grpc.h: In member function ‘virtual uint8_t* flatbuffers::grpc::SliceAllocator::reallocate_downward(uint8_t*, size_t, size_t, size_t, size_t)’:
src/vendor/flatbuffers/include/flatbuffers/grpc.h:124:12: warning: function may return address of local variable [-Wreturn-local-addr]
  124 |     return new_p;
      |            ^~~~~
src/vendor/flatbuffers/include/flatbuffers/grpc.h:119:19: note: declared here
  119 |     ::grpc::Slice new_slice = ::grpc::Slice(new_size);
      |                   ^~~~~~~~~

I updated the MR to return the pointer to the _slice.begin() which should be the same as new_p and allowing the compiler to understand that the address is not out of scope

Copy link
Collaborator

@aardappel aardappel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this seems strange that this is required for the compiler, but I guess we'll merge for now.

@aardappel aardappel enabled auto-merge (squash) August 23, 2025 16:49
@fliiiix
Copy link
Contributor Author

fliiiix commented Aug 24, 2025

Thanks, I assume at some point diagnostics will become better but for now i guess the compiler sees the pointer and new_slice and both are going out of scope and it misses the fact that this memory is now owned by slice_

@mustiikhalil mustiikhalil disabled auto-merge August 24, 2025 07:10
@mustiikhalil
Copy link
Collaborator

Sorry was reading from the phone and miss clicked

@mustiikhalil mustiikhalil enabled auto-merge (squash) August 24, 2025 07:11
@fliiiix
Copy link
Contributor Author

fliiiix commented Aug 26, 2025

@aardappel i don't think any failed CI jobs are related to this change?

@aardappel aardappel disabled auto-merge August 27, 2025 21:50
@aardappel aardappel enabled auto-merge (squash) August 27, 2025 21:51
@dbaileychess dbaileychess disabled auto-merge August 28, 2025 20:57
@aardappel aardappel merged commit b87d04a into google:master Aug 28, 2025
50 checks passed
@fliiiix fliiiix deleted the bugfix/ignore-wrong-warning branch August 29, 2025 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants