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 @safe and pure to enforce family #263

Merged
merged 2 commits into from
Sep 24, 2011
Merged

Conversation

9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Sep 16, 2011

By fixing issue 5750, we can add more pure and @safe to enforce family.

{
if (!value) bailOut(file, line, msg);
if (!value)
bailOut(file, line, msg);
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if there wouldn't be so many of these kind of changes, especially since we don't have a style policy on how to format short bodies.

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. I have reverted that indentaitons. It is not my wish to be rejected this change because of the trivial.
But, I dislike not indented code even if they are short bodies.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't be in a position to reject your changes anyway, and it's not like I would have a problem with splitting it to two lines, but imho arbitrary style changes for things not covered by the style guide should always be kept at a minimum, since it clutters up diffs and makes code needlessly hard to review.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that rewrites are fine when code is actually being revised, but minor changes to code that isn't being changed for other reasons shouldn't be happening.

@jmdavis
Copy link
Member

jmdavis commented Sep 17, 2011

Well, it does appear to work with arguments to enforce which aren't @safe or pure, which is essentially the same as if the parameter weren't lazy. As long as that behavior is going to stay as-is, then I think that this is a positive change. Even if @safe and pure shouldn't be necessary, they don't actually hurt, and making enforce pure will be a major boon for purity. It's one of the major reasons that stuff in Phobos can't be pure (the other really big one being that Appender can't be used in pure functions).

So, as long as we're sure that the behavior of lazy here is correct and that compiler changes aren't going to cause problems for arguments which aren't really pure or @safe, I think that these changes should be merged in even if @safe and pure shouldn't be necessary.

@9rnsr
Copy link
Contributor Author

9rnsr commented Sep 22, 2011

Now issue 6690 was fixed, then we don't need the @safe pure annotations except for the addition to the bailOut.

Which is better, adding @safe pure for descriptive documentation, or removing them with relying on @safe pure inference?

@jmdavis
Copy link
Member

jmdavis commented Sep 22, 2011

I would argue that we should use @safe, pure, and nothrow on templated functions when the intention is that they always have that attribute regardless of what arguments they're instantiated with. It's then clear that they are going to always have that attribute, and it could catch bugs in cases where it somehow becomes possible for them not to have that attribute when inferred. I think that we should leave inferrence for cases where we don't know whether the function will be inferred to have that particular attribute or not.

@@ -350,7 +350,7 @@ enforce(line.length, "Expected a non-empty line."));
--------------------
+/
T enforce(T, string file = __FILE__, int line = __LINE__)
Copy link
Member

Choose a reason for hiding this comment

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

Please fix all of the versions of enforce and its related functions so that line is size_t. I realize that it's not your fault in all cases (like this line), since it was int before in some cases, but it's a bit of a hodgepodge at the moment - including in your changes. It should be size_t in all cases, since __LINE__ is size_t. So, please make the change. Once that's done, I think that we should merge these changes in.

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. I changed them into size_t.

jmdavis added a commit that referenced this pull request Sep 24, 2011
@jmdavis jmdavis merged commit 0bf4ed8 into dlang:master Sep 24, 2011
@jmdavis
Copy link
Member

jmdavis commented Sep 24, 2011

Merged.

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.

3 participants