Skip to content

Conversation

@mhucka
Copy link
Member

@mhucka mhucka commented Jan 4, 2025

This file produces many warnings of the form

tensorflow_quantum/core/src/circuit_parser_qsim.cc:185:8:
warning: variable 'unused' set but not used [-Wunused-but-set-variable]
  185 |   bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0);
      |        ^

Inspecting the code reveals that variable unused is indeed set but never used. This construct was used in many places in this file, and always involving SimpleAtoi.

I replaced the code with a simple alternative that tests the returned value from SimpleAtoi, in a way that mirrors what was already done in one place in the file where unused was not used. This approach follows the recommendations from the Abseil docs, too.

Since this change means that new exceptions may be returned if the status from SimpleAtoi is not okay, it also needed the addition of test cases in circuit_parser_qsim_test.cc.

A final change is to fix a couple of warnings involving int versus unsignned int.

mhucka added 4 commits January 4, 2025 20:55
This file produces many warnings of the form

```
tensorflow_quantum/core/src/circuit_parser_qsim.cc:185:8:
warning: variable 'unused' set but not used [-Wunused-but-set-variable]
  185 |   bool unused = absl::SimpleAtoi(op.qubits(0).id(), &q0);
      |        ^
```

Inspecting the code reveals that variable `unused` is indeed set but
never used. I replaced the code with simple testing of the return
values, similar to what is used in one place in the file, and in a way
that follows the recommendations from the Abseil docs.
@mhucka mhucka marked this pull request as ready for review January 4, 2025 23:12
@mhucka mhucka added kind/chore Maintenance & housekeeping tasks area/health Involves general matters of project configuration, health, maintenance, and similar concerns labels Jan 5, 2025
@mhucka mhucka self-assigned this Jan 5, 2025
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looking at

tensorflow::Status ResolveQubitIds(

I remember our process for getting to qsim circuits is this:

  1. Replace Grid/Line Qubit representations in the circuit and paulisum protos with integer indices that all line up and are well ordered across the batch.
  2. Forward this partially processed proto here to unpack into qsim circuit structs.
  3. Actually simulate the circuit placing results back into tensors in the right order.

So whenever I call into any of these gate constructors it is a requirement that all the proto messages contain integers. I might be more inclined to use assertions (or nothing at all and just (void) it out) here.

@mhucka
Copy link
Member Author

mhucka commented Jan 10, 2025

Thanks for the PR. Looking at

tensorflow::Status ResolveQubitIds(

I remember our process for getting to qsim circuits is this:

  1. Replace Grid/Line Qubit representations in the circuit and paulisum protos with integer indices that all line up and are well ordered across the batch.
  2. Forward this partially processed proto here to unpack into qsim circuit structs.
  3. Actually simulate the circuit placing results back into tensors in the right order.

So whenever I call into any of these gate constructors it is a requirement that all the proto messages contain integers. I might be more inclined to use assertions (or nothing at all and just (void) it out) here.

Oh, I understand now: due to the other steps around these calls to SimpleAtoi, the calls will always get valid values, therefore the returns don't need to be tested.

I lean towards using void for these cases, mainly due to having had the principle of "assert is for test code, not production code" drilled into me for too long …

Per discussion with Michael B. on an earlier version of this PR, I
replaced the assignments with simple (void) casts.
I guess the style is "(void)foo" and not "(void) foo".
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

LGTM

@mhucka mhucka merged commit 452a212 into tensorflow:master Jan 13, 2025
6 of 7 checks passed
@mhucka mhucka mentioned this pull request Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/health Involves general matters of project configuration, health, maintenance, and similar concerns kind/chore Maintenance & housekeeping tasks

Development

Successfully merging this pull request may close these issues.

2 participants