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

Fixing warnings uncovered by Clang 15.0 on macOS 14.7.1. #298

Closed
wants to merge 2 commits into from

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Nov 26, 2024

Fixing the build errors seen in e.g. this GitHub build output which says

/Users/runner/work/mps/mps/code/trans.c:154:9: error: variable 'added' set but not used [-Werror,-Wunused-but-set-variable]
(2 failures)
  Count added = 0;

The "added" count variable has been around since Perforce changelist 178798 in 2012, before transforms were released to Configura in MPS 1.110. It does not appear to ever have been used for anything.

…arning when building on macOS 14.7.1 with Clang 15.0.
@rptb1
Copy link
Member Author

rptb1 commented Nov 27, 2024

According to the build output, we have a longer list of enabled warnings in the MPS Xcode project than in our make files. However, copying that list to ll.gmk and doing, e.g.

CC=clang-15 make -f lii6ll.gmk VARIETY=cool NOISY=true clean lii6ll/cool/trans.o

does not produce the build error on Ubuntu 22.04. So something is different about the Clang 15.0 on Ubuntu vs macOS/Xcode.

rb@kiwi:~/git/mps/code$ clang-15 --version
Ubuntu clang version 15.0.7
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

It's a shame, because it would be nice to get an early warning of macOS issues when working on Linux!

@rptb1 rptb1 self-assigned this Nov 27, 2024
@rptb1 rptb1 added build Problems with builds and build automation low risk This work is or would be of low risk of introducing defects. labels Nov 27, 2024
@rptb1 rptb1 marked this pull request as ready for review November 27, 2024 11:32
@rptb1
Copy link
Member Author

rptb1 commented Nov 27, 2024

Executing proc.review.express.

  1. Start time 11:33.
  2. Applied entry.universal and entry.impl. The build now succeeds on GitHub's macos-latest runner.
  3. Calling in @thejayps . There will be a short delay.

Copy link
Member Author

@rptb1 rptb1 left a comment

Choose a reason for hiding this comment

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

Executing proc.review.check.

  1. Start time 12:37.
  2. No defects found.
  3. End time 12:44.

@thejayps
Copy link
Contributor

Checking: m1: assume.parked in line 162, code then goes on to AVER arena is clamped, which is at face value a different state to parked. The following line AVER(arena->busyTraces == TraceSetEMPTY); then does check that no traces are running, so the arena is parked. But there is a clarity issue here, where we have 2 distinct states and appear to be checking for the wrong one.

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

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

ignore ... comment below not intended for this pr

"Checking - no defects found after 6 minutes"

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

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

Checking - no defects found after 6 minutes

@rptb1
Copy link
Member Author

rptb1 commented Nov 27, 2024

Continuing proc.review.express.
4. Made a minor clarifying edit in pair with @thejayps.
5. Waited for automated checks to complete as expected.
6. proc.review.exit.pass: Revised change passed.
7. proc.review.exit.calc: about 40 minutes used.

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

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

@rptb1 has done exit.pass

rptb1 added a commit that referenced this pull request Nov 27, 2024
@rptb1
Copy link
Member Author

rptb1 commented Nov 27, 2024

Executing proc.merge.pull-request.

  1. Start time 13:20.
  2. Merged and ran local ci tests before pushing.
  3. End time 13:27.

@rptb1
Copy link
Member Author

rptb1 commented Nov 27, 2024

Hmm, GitHub didn't close this PR when we merged the changes, resulting in 9fd0577 . During merge I did proc.merge.pull-request section 5 step 6, git pull --rebase=merges github, in order to catch up to master. This did not do what we expect, because it changes the hashes of the commits. So the procedure doesn't produce the intended effect. This is input to #228 .

@rptb1 rptb1 closed this Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Problems with builds and build automation low risk This work is or would be of low risk of introducing defects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants