Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add more utils #661

Merged
merged 5 commits into from
Nov 15, 2013
Merged

Add more utils #661

merged 5 commits into from
Nov 15, 2013

Conversation

denis-sh
Copy link
Contributor

@denis-sh denis-sh commented Nov 7, 2013

As a result of safer interface few bugs are fixed and the ground is prepared for type-safe console output in druntime without calling any C I/O routines.

… functions.

New interface is safer. Also it fixes bug in `_d_arraycopy` where buffer was too small for 64 bit `size_t`.
New interface is safer. Also it fixes bug in `_d_arrayctor` which didn't check for array overlap.
@MartinNowak
Copy link
Member

It's a bugfix. buf must be sopce as noted in commit description. Here is the reason: object_@1370.

Since when is in equal to scope? I only saw that proposed in discussions about reference escaping.

@MartinNowak
Copy link
Member

I like some parts of this pull request, others not so much. In any case it's quite a lot of different stuff for one pull.

  • The uintToString change is good.
  • The format function adds a lot of template bloat.
  • Using ^ as format char looks weird.
  • Replacing the array checks with dedicated functions is good.
  • I have some reservations against putting exceptions in the array functions.
    As @braddr we'd ideally expose the contracts to the compiler.
    But currently this is not done and thus using exceptions binds the array ops
    to the GC which makes them unusable before the runtime is initialized and on targets
    which don't support a GC.

@denis-sh
Copy link
Contributor Author

Since when is in equal to scope?

By definition. According to Function Parameters "in equivalent to const scope".

I have some reservations against putting exceptions in the array functions...

As Errors are unrecoverable anyway we really don't need GC allocations here. So I will change enforce*Conformable functions to just halt with assert(0) or throwing without GC.

The format function adds a lot of template bloat.

It also gains friendly API for runtime developers and performance improvement. How much exactly do we loose on template bloat?

@MartinNowak
Copy link
Member

It also gains friendly API for runtime developers and performance improvement. How much exactly do we loose on template bloat?

It's not so bad, I've seen a few 10kBs. But if the pull was separately we could audit for possible savings.

@MartinNowak
Copy link
Member

As Errors are unrecoverable anyway we really don't need GC allocations here. So I will change enforce*Conformable functions to just halt with assert(0) or throwing without GC.

It isn't of utmost importance right now because the array functions are X86 desktop only.
I hope that we can replace the array ops with a templated version (see discussion) which would allow to rely on contracts.

@denis-sh
Copy link
Contributor Author

Formatting commit excluded. I'd like also enforce*Conformable functions to be as is as it's consistent with the rest of druntime. In the future we can change the runtime to work partially without GC but for now it alrady allocate on Errors so such design change goes to another pull.

@MartinNowak
Copy link
Member

LGTM, waiting for Autotester.

@braddr
Copy link
Member

braddr commented Nov 15, 2013

Wanna try auto the new auto-merging feature? (checkout my email to dmd-internal and phobos)

@MartinNowak
Copy link
Member

Wanna try auto the new auto-merging feature? (checkout my email to dmd-internal and phobos)

Yep I did, it just so happens that it was the next mail.

MartinNowak added a commit that referenced this pull request Nov 15, 2013
@MartinNowak MartinNowak merged commit 9e510eb into dlang:master Nov 15, 2013
@MartinNowak
Copy link
Member

Wanna try auto the new auto-merging feature?

Works 👍.

@denis-sh
Copy link
Contributor Author

It's not so bad, I've seen a few 10kBs. But if the pull was separately we could audit for possible savings.

Opened #662.

@denis-sh
Copy link
Contributor Author

Since when is in equal to scope?

By definition. According to Function Parameters "in equivalent to const scope".

Also the compiler doesn't respect docs and e.g. allocates closures when delegates are passed with in direction. Filed Issue 11602.

@dnadlinger
Copy link
Member

Hm, what was the rationale for moving towards always checking array op validity (also in release builds)?

@MartinNowak
Copy link
Member

Hm, what was the rationale for moving towards always checking array op validity (also in release builds)?

The current extern(C) interface doesn't allow to distinguish debug and release builds.
http://d.puremagic.com/issues/show_bug.cgi?id=8650#c8
Overlapping checks are not that expensive.

@CyberShadow
Copy link
Member

Possible regression: https://issues.dlang.org/show_bug.cgi?id=14783

Does this PR add overlap checking functionality or refactor existing one? If the latter, the refactoring introduced a regression.

@denis-sh
Copy link
Contributor Author

Possible regression: https://issues.dlang.org/show_bug.cgi?id=14783

It's an enhancement, not a regression.

Does this PR add overlap checking functionality or refactor existing one? If the latter, the refactoring introduced a regression.

This pull fixes overlap checking. Previously cases like x[] *= x[] we accepted in release builds and rejected in debug ones.

@CyberShadow
Copy link
Member

cases like x[] *= x[] we accepted in release builds and rejected in debug ones.

I tested multiple D versions and in none of them did this code raise an error in non-release mode.
Edit: D versions predating this pull, i.e. pre-2.065

@denis-sh
Copy link
Contributor Author

cases like x[] *= x[] we accepted in release builds and rejected in debug ones.

I tested multiple D versions and in none of them did this code raise an error in non-release mode.

As you see in a6aa6ee, there were some checks, but only in debug build of a runtime. Also "cases like" doesn't mean this particular case, maybe for this exact type and this exact operator the check weren't even present in precondition block.

@CyberShadow
Copy link
Member

Regression or not applies to the final effect, not the nature of the change. If the checks were in a debug-only build of Druntime, they were effectively not there at all as we don't ship a debug runtime and almost no one builds one for themselves. If a well-intended and generally correct change broke valid code, it's still a regression. See issue discussion for whether this code is valid or not?

@denis-sh
Copy link
Contributor Author

See issue discussion for whether this code is valid or not?

This code is not valid. Overlapping operations are not supported.

@CyberShadow
Copy link
Member

OK.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants