Skip to content

Ensure that the MPS builds with Clang 10.#52

Merged
rptb1 merged 1 commit intomasterfrom
branch/2021-01-10/implicit-conversion
Jan 16, 2021
Merged

Ensure that the MPS builds with Clang 10.#52
rptb1 merged 1 commit intomasterfrom
branch/2021-01-10/implicit-conversion

Conversation

@gareth-rees
Copy link
Copy Markdown
Member

@gareth-rees gareth-rees commented Jan 10, 2021

Clang 10 turns on -Wimplicit-int-float-conversion, and on 64-bit platforms this issues a warning that implicit conversions to double from Size and unsigned long "may lose precision".

This commit adds casts to (double) for all such conversions. The loss of precision is either impossible in practice (because a double can represent all integers up to 2**53, which is about 9 petabytes, well beyond the addressing capabilities for current CPUs), or else acceptable, because we are accumulating an approximate quantity like "collection work" or "fill size" (that has to cope with loss of precision due in any case), or computing a threshold like the spare commit limit where it is acceptable for it to be somewhat approximate.

Fixes #51 (MPS does not build with clang 10 on Ubuntu 20 x86_64).

@gareth-rees gareth-rees requested review from UNAA008 and rptb1 January 10, 2021 10:39
Clang 10 turns on -Wimplicit-int-float-conversion, and on 64-bit
platforms this issues a warning that implicit conversions to double
from Size and unsigned long "may lose precision".

This commit adds casts to (double) for all such conversions. The loss
of precision is either impossible in practice (because a double can
represent all integers up to 2**53, which is about 9 petabytes, well
beyond the addressing capabilities for current CPUs), or else
acceptable, because we are accumulating an approximate quantity like
"collection work" or "fill size" (that has to cope with loss of
precision due in any case), or computing a threshold like the spare
commit limit where it is acceptable for it to be somewhat approximate.
@gareth-rees gareth-rees force-pushed the branch/2021-01-10/implicit-conversion branch from 58cbf33 to 9b55657 Compare January 10, 2021 10:42
@rptb1
Copy link
Copy Markdown
Member

rptb1 commented Jan 10, 2021

I think that, rather than adding lots of casts, we should add a macro/inline function that expresses the intention, documented with your conditions above. We want everyone who comes across the situation to ponder whether the conditions are true, rather than insert casts to make warnings go away. Perhaps in code/misc.h

@rptb1
Copy link
Copy Markdown
Member

rptb1 commented Jan 10, 2021

9b55657 builds without error on the VM setup that I used to generate #51

@gareth-rees
Copy link
Copy Markdown
Member Author

gareth-rees commented Jan 10, 2021

I think that, rather than adding lots of casts, we should add a macro/inline function that expresses the intention, documented with your conditions above. We want everyone who comes across the situation to ponder whether the conditions are true, rather than insert casts to make warnings go away. Perhaps in code/misc.h

I think this is best done by opening a new issue and fixing it in another pull request — there are more than 250 casts to (double) and it takes some thought to figure out the intention for each one. I don't think it is appropriate for the perfect (investigating and justifying every cast in the code base) to be the enemy of the good (compiling under Clang 10).

@rptb1
Copy link
Copy Markdown
Member

rptb1 commented Jan 10, 2021

there are more than 250 casts to (double) and it takes some thought to figure out the intention for each one.

I believe every cast you have added in this change is a valid instance. In other words, it would be fine to replace every cast you have added with a reference to the same documented intention for the purpose of fixing this issue. I'm just concerned with the future.

@rptb1
Copy link
Copy Markdown
Member

rptb1 commented Jan 10, 2021

@gareth-rees @UNAA008 and I discussed this in a meeting. We agreed that super stringent future-proofing of pull requests is probably not appropriate for an open community project, which means changing our practices somewhat. In this case, @gareth-rees should merge his change, and we should open a new issue dealing with the fact that the change fails to express intent in a way that could cause future quality problems. Git and GitHub will maintain the necessary audit trail via git blame and the issue can be fixed up in future at a lower priority. See also #54, “MPS repository is not set up for public contributors”. We are learning!

@rptb1 rptb1 merged commit 8797bab into master Jan 16, 2021
rptb1 added a commit that referenced this pull request Mar 8, 2023
…version

Ensure that the MPS builds with Clang 10.

(cherry picked from commit 8797bab)
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Sep 17, 2025
Ensure that the MPS builds with Clang 10.

GitHub-reference: Ravenbrook/mps#52
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.

MPS does not build with clang 10 on Ubuntu 20 x86_64

2 participants