Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename shadowed variables #13037

Closed
4kangjc opened this issue Jun 13, 2023 · 10 comments
Closed

Rename shadowed variables #13037

4kangjc opened this issue Jun 13, 2023 · 10 comments

Comments

@4kangjc
Copy link
Contributor

4kangjc commented Jun 13, 2023

What version of protobuf and what language are you using?
Version: main
Language: C++

What operating system (Linux, Windows, ...) and version?
MacOS

What runtime / compiler are you using (e.g., python version or gcc version)

# Compile failed
$ bazel build --copt='-Wshadow-all' :protoc :protobuf
$ clang --version
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

What did you expect to see
Shadowed variables can cause readability issues. Ensure a shadowed
variable isn't used in header files which may be used in a dependent
project that explicitly disables them.
bazel build --copt='-Wshadow-all' :protoc :protobuf executed successfully.

@4kangjc 4kangjc added the untriaged auto added to all issues by default when created. label Jun 13, 2023
@4kangjc 4kangjc changed the title Rename a shadowed variable Rename shadowed variables Jun 13, 2023
@deannagarcia
Copy link
Member

Thanks for reporting this issue! I was able to successfully reproduce it and I see errors with shadowed variables.

For future record, here is an example of the logs:

bazel-out/k8-fastbuild/bin/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/serial_arena.h: In constructor 'google::protobuf::internal::ArenaBlock::ArenaBlock(google::protobuf::internal::ArenaBlock*, size_t)':
bazel-out/k8-fastbuild/bin/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/serial_arena.h:71:39: error: declaration of 'size' shadows a member of 'google::protobuf::internal::ArenaBlock' [-Werror=shadow]
   71 |   ArenaBlock(ArenaBlock* next, size_t size)
      |                                ~~~~~~~^~~~
bazel-out/k8-fastbuild/bin/src/google/protobuf/_virtual_includes/protobuf_lite/google/protobuf/serial_arena.h:86:16: note: shadowed declaration is here
   86 |   const size_t size;
      |                ^~~~

@fowles
Copy link
Contributor

fowles commented Jun 13, 2023

I'm sorry, but we don't support compiling with this warning enabled. The best thing you can do here is to modify port_def.inc. Feel free to send a PR

@4kangjc
Copy link
Contributor Author

4kangjc commented Jun 14, 2023

I'm sorry, but we don't support compiling with this warning enabled. The best thing you can do here is to modify port_def.inc. Feel free to send a PR

OK

@4kangjc
Copy link
Contributor Author

4kangjc commented Jun 14, 2023

image

I feel that the naming of this variable called options is not very standardized.

@4kangjc
Copy link
Contributor Author

4kangjc commented Jun 14, 2023

image

Maybe we need to rename i to j?

@fowles
Copy link
Contributor

fowles commented Jun 14, 2023

Renaming existing violations won't stop backsliding in the future, I would strongly prefer a PR that suppresses the warnings.

@4kangjc
Copy link
Contributor Author

4kangjc commented Jun 14, 2023

OK, I see.

4kangjc added a commit to 4kangjc/protobuf that referenced this issue Jun 14, 2023
4kangjc added a commit to 4kangjc/protobuf that referenced this issue Jun 15, 2023
fowles pushed a commit to fowles/protobuf that referenced this issue Jun 16, 2023
@fowles
Copy link
Contributor

fowles commented Jun 16, 2023

started #13088 to cherry pick this back to 23.x. We won't spin a release specifically for this, but it will be picked up in the next 23.x point release

@4kangjc
Copy link
Contributor Author

4kangjc commented Jun 16, 2023

started #13088 to cherry pick this back to 23.x. We won't spin a release specifically for this, but it will be picked up in the next 23.x point release

OK, I see.

@fowles
Copy link
Contributor

fowles commented Jun 16, 2023

Don't worry, we will spin a point release for #13075 once it lands, so I don't think you have long to wait

@googleberg googleberg removed the untriaged auto added to all issues by default when created. label Feb 17, 2024
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 a pull request may close this issue.

4 participants