Skip to content

Comments

Rewrite synopsis example of std.exception#6081

Merged
MartinNowak merged 1 commit intodlang:stablefrom
wilzbach:rewrite-exception-synopsis
Feb 27, 2018
Merged

Rewrite synopsis example of std.exception#6081
MartinNowak merged 1 commit intodlang:stablefrom
wilzbach:rewrite-exception-synopsis

Conversation

@wilzbach
Copy link
Contributor

It didn't compile (see #6080).

  • converted to a module unittest
  • fixed the example, s.t. it compiles
  • made it to a full synposis by adding more cool functions from std.exception

@wilzbach wilzbach requested a review from andralex as a code owner January 28, 2018 02:35
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

std/exception.d Outdated
{
mixin basicExceptionCtors;
}
assertThrown!MeaCulpa(enforce!MeaCulpa(0 == 1, "0 is not 1."));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just show off construction

auto e = MeaCulpa("diagnostic message");
assert(e.msg == "diagnostic message");
assert(e.file == __FILE__);
assert(e.line == __LINE__ - 3);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but I used throw. Otherwise there would be no point of using basicExceptionCtors ...

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Please consider @JackStouffer's suggestion.

@wilzbach wilzbach force-pushed the rewrite-exception-synopsis branch from 9227b5b to 78cd967 Compare January 28, 2018 03:44
std/exception.d Outdated
module std.exception;

/// Synopis
unittest
Copy link
Contributor

Choose a reason for hiding this comment

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

@system

std/exception.d Outdated
assert(e.file == __FILE__);
assert(e.line == __LINE__ - 3);

// doesPointTo can be used to check whether the source contains pointers / references
Copy link
Contributor

Choose a reason for hiding this comment

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

"The source"? Also, it doesn't just check for pointers/references, but ones pointing into the target. Perhaps this is best removed from the synopsis? The function doesn't really fit into std.exception anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function doesn't really fit into std.exception anyway.

Excellent point (I was wondering about the same). Suggestions?
Probably stdx.allocator would probably be the best fit...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyhow, I removed it for now and opened a Bugzilla issue: https://issues.dlang.org/show_bug.cgi?id=18387

@wilzbach wilzbach force-pushed the rewrite-exception-synopsis branch 2 times, most recently from 2e7ee25 to fc1e7ad Compare February 8, 2018 23:07
@wilzbach wilzbach force-pushed the rewrite-exception-synopsis branch from fc1e7ad to 36f80d3 Compare February 25, 2018 08:39
@wilzbach wilzbach changed the base branch from master to stable February 25, 2018 08:39
@wilzbach
Copy link
Contributor Author

Rebased to stable, s.t. this gets deployed soon to the documentation.
Looks like nothing is blocking this and it has two approvals. Anyone around to do the honor?

(I moved the discussion of the ill-fit doesPointTo to https://issues.dlang.org/show_bug.cgi?id=18387)

@MartinNowak MartinNowak merged commit 9d00e0d into dlang:stable Feb 27, 2018
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.

6 participants