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

add root/checkedint.h and .c #3800

Merged
merged 1 commit into from Aug 19, 2014
Merged

Conversation

WalterBright
Copy link
Member

This is a direct translation of druntime's core.checkedint to C++. The idea is to replace the several ad-hoc overflow checks in dmd with these. Even includes unit tests!

@ibuclaw
Copy link
Member

ibuclaw commented Jul 23, 2014

Cool. How goes 2.066? Is it currently branched and taking cherry pickings from master?

I would initially suggest wait until after release to make sure this doesn't accidently get pulled in.

@yebblies
Copy link
Member

Does this exactly match the core.checkedint api?

@WalterBright
Copy link
Member Author

@ibuclaw this is for 2.067, 2.066 is branched

@yebblies yes

@yebblies
Copy link
Member

  1. These use a weird mix of int64_t etc and unsigned. I suggest you change it to always use the stdint types.
  2. Why do you need the UINT64_MAX hackery?
  3. Did your unrelated changes to posix.mak break Brad's recent pulls?

With the api matching, I will completely drop this file for ddmd.

@WalterBright
Copy link
Member Author

  1. I found too many problems with stdint types. I gave up on it and went with unsigned.
  2. What do you mean?
  3. ???

mkdir -p $(INSTALL_DIR)/$(OS)/$(bin_dir)
cp dmd $(INSTALL_DIR)/$(OS)/$(bin_dir)/dmd
cp ../ini/$(OS)/$(bin_dir)/dmd.conf $(INSTALL_DIR)/$(OS)/$(bin_dir)/dmd.conf
cp ../ini/$(OS)/$(bin_dir)/dmd.conf $(INSTALL_DIR)/bin/dmd.conf
Copy link
Member

Choose a reason for hiding this comment

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

What is going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

merge problem. Fixed.

@yebblies
Copy link
Member

I found too many problems with stdint types. I gave up on it and went with unsigned.

Can you be more specific? Those types should be straightforward aliases.

@WalterBright
Copy link
Member Author

The macro constants are typed incorrectly with dmc.

@yebblies
Copy link
Member

The macro constants are typed incorrectly with dmc.

That's really worrying, but anyway - what does this have to do with using unsigned over uin32_t?

@WalterBright
Copy link
Member Author

UINT32_MAX is the wrong type, so would have to be constantly cast, or the overloads in the unittests are incorrect.

@yebblies
Copy link
Member

I have no idea what you mean. You've just redefined UINT32_MAX because it was the wrong type, and now you're saying you can't use uint32_t because UINT32_MAX is STILL the wrong type?

@WalterBright
Copy link
Member Author

Also, dmd throughout uses int and unsigned, not int32_t and uint32_t, so this matches. unsigned long long, however, is a non-portable mess, so I use stdint for that.

@WalterBright
Copy link
Member Author

This experience makes me long (!) for D's predictable integral types.

@yebblies
Copy link
Member

Also, dmd throughout uses int and unsigned, not int32_t and uint32_t, so this matches. unsigned long long, however, is a non-portable mess, so I use stdint for that.

So is there a technical limitation that's stopping you from using int32_t or not?

This experience makes me long (!) for D's predictable integral types.

You could always put this off until after the switch to D.

@WalterBright
Copy link
Member Author

int32_t is simply pointless. DMD uses int, it needs to overflow check on ints, not int32_t's.

We could put off everything until after the switch to D. But this addition shouldn't affect ddmd in any material way, you can just skip these files.

@andralex
Copy link
Member

Auto-merge toggled on

andralex added a commit that referenced this pull request Aug 19, 2014
@andralex andralex merged commit a07ff94 into dlang:master Aug 19, 2014
@WalterBright WalterBright deleted the checkedint branch August 19, 2014 22:19
@Orvid
Copy link
Contributor

Orvid commented Aug 19, 2014

This failed to add the new files to the VC projects.

@yebblies
Copy link
Member

This failed to add the new files to the VC projects.

VC projects are not officially supported. Feel free to make a pull request to update them.

@quickfur
Copy link
Member

This pull introduced a compile error on Linux/x86_64 when building with make DEBUG=1:

g++ -m64 -c -Wno-deprecated -Wstrict-aliasing -fno-exceptions -fno-rtti -D__pascal= -DMARS=1 -DTARGET_LINUX=1 -DDM_TARGET_CPU_X86=1  -g -g3 -DDEBUG=1 -DUNITTEST -DDMDV2=1 -Iroot -Itk -Ibackend -MMD -MF e2ir.deps e2ir.c
In file included from root/checkedint.c:30:0:
root/checkedint.c: In function ‘void unittest2()’:
root/checkedint.c:98:35: error: call of overloaded ‘adds(long long int, long long int, bool&)’ is ambiguous
     assert(adds(2LL, 3LL, overflow) == 5);
                                   ^
root/checkedint.c:98:35: note: candidates are:
root/checkedint.c:56:5: note: int adds(int, int, bool&)
 int adds(int x, int y, bool& overflow)
     ^
root/checkedint.c:85:9: note: int64_t adds(int64_t, int64_t, bool&)
 int64_t adds(int64_t x, int64_t y, bool& overflow)
         ^
In file included from root/checkedint.c:30:0:
root/checkedint.c:102:48: error: call of overloaded ‘adds(long long int, long long int, bool&)’ is ambiguous
     assert(adds(INT64_MIN + 1LL, -1LL, overflow) == INT64_MIN);
                                                ^
root/checkedint.c:102:48: note: candidates are:
root/checkedint.c:56:5: note: int adds(int, int, bool&)
 int adds(int x, int y, bool& overflow)
     ^
root/checkedint.c:85:9: note: int64_t adds(int64_t, int64_t, bool&)
 int64_t adds(int64_t x, int64_t y, bool& overflow)
         ^
In file included from root/checkedint.c:30:0:
root/checkedint.c:109:35: error: call of overloaded ‘adds(long long int, long long int, bool&)’ is ambiguous
     assert(adds(0LL, 0LL, overflow) == 0);
                                   ^
root/checkedint.c:109:35: note: candidates are:
root/checkedint.c:56:5: note: int adds(int, int, bool&)
 int adds(int x, int y, bool& overflow)
     ^
root/checkedint.c:85:9: note: int64_t adds(int64_t, int64_t, bool&)
 int64_t adds(int64_t x, int64_t y, bool& overflow)
         ^

(there are many pages of similar errors that follow this).

@quickfur
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants