Skip to content

improve test for the -compat-32 flag#1161

Merged
mshinwell merged 2 commits intoocaml:trunkfrom
hhugo:compat32-test
May 8, 2017
Merged

improve test for the -compat-32 flag#1161
mshinwell merged 2 commits intoocaml:trunkfrom
hhugo:compat32-test

Conversation

@hhugo
Copy link
Copy Markdown
Contributor

@hhugo hhugo commented May 7, 2017

No description provided.

Copy link
Copy Markdown
Member

@dra27 dra27 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 this - it does indeed sort out the Windows failure.

I'd prefer using $(OCAMLC) -config | tr -d '\r' | grep "^word_size:" | sed -e "s/.*: //" to having to build a program to test the output (unfortunately you can't search for ARCH_SIXTYFOUR in m.h without first pre-processing it). At some point, I will make $(OCAMLC) -config word_size a thing!

run:
@printf " ... testing -compat-32"
@$(OCAMLC) -compat-32 -c a.ml 2>&1 | xargs echo ${} > test.result
@$(OCAMLC) -compat-32 -c a.ml > test.result 2>&1 | true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

|| true, rather than a pipe (and two more cases below)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -1,2 +1,2 @@
let () =
if Sys.word_size = 64 then exit 0 else exit 1
if Sys.int_size >= 63 then exit 0 else exit 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change isn't necessary - the removal of the xargs call is sufficient for Windows. Personally the original is fine, but if it's going to be an inequality, shouldn't it be > 32?

However, I also think this is a bit too heavyweight - could you just grep the output of $(OCAMLC) -config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't know word_size and int_size were displayed during -config - updated.

clean test for -compat-32, CR
@mshinwell
Copy link
Copy Markdown
Contributor

I'm going to merge this now, since @dra27 has looked at it, and I think the failure this fixes is breaking other PRs. I tend to think the simple grep for word_size which is currently here probably suffices; @dra27 if you feel strongly please talk to @hhugo directly.

@mshinwell mshinwell merged commit e789651 into ocaml:trunk May 8, 2017
@hhugo hhugo deleted the compat32-test branch March 26, 2018 22:47
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 21, 2023
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: cuihtlauac <cuihtlauac@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants