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 std.string.outdent #156

Closed
wants to merge 19 commits into from
Closed

add std.string.outdent #156

wants to merge 19 commits into from

Conversation

Abscissa
Copy link
Contributor

No description provided.

return arr;
}

private bool ctfe_isWhite(dchar c)
Copy link
Contributor

Choose a reason for hiding this comment

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

... but std.uni.isWhite is CTFE-able.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... but std.uni.isWhite is CTFE-able.

Hmm, it wasn't when I originally wrote it in my own libs. Looks like it is
now.

(not is from std.functional)
The cast is unnecessary. All char types are implicitly convertible to
dchar.

Thanks, will fix.

std.range.join is CTFE-able.

It isn't in 2.054. Though it looks like it is in the git version I grabbed
yesterday. Maybe I'll use it and submit a separate pull request for some
CTFEability tests to make sure stuff likely to be needed at compile time
stays CTFEable... IIRC, Don did say on the NG that after 2.054 there
shouldn't be more CTFEability regressions.

@kennytm
Copy link
Contributor

kennytm commented Jul 24, 2011

Note. This pull request used a lot of CTFE version of existing functions. Ideally, none of them should exist. The non-CTFE-able functions currently are:

  • std.array.split, which is not CTFE-able only because of std.array.appender, which is due to bug 6374
  • std.string.strip, stripLeft and stripRight, which are due to bug 3512.

@jmacdonagh
Copy link
Contributor

At the very least the applicable bugs should be noted in a comment for each of the ctfe_* routines. That way when they're fixed it's easy to find these routines and fix them.


S outdent(S)(S str) if(isSomeString!S)
{
if(str == "")
Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't matter that much, but In general, I'd be inclined to argue that empty should be used rather than == "".

@jmdavis
Copy link
Member

jmdavis commented Jul 25, 2011

According to Don, bug 3512 should be fixed by the next release, so your CTFE versions of the strip functions shouldn't be needed for much longer.

I don't know about bug 6374 though. Not being able to have Appender be CTFEable is a big problem. As functions which currently only take string are templated to take any string type, the number of string functions using Appender is likely to increase. But since there's a trivial workaround for fixing Appender, even if the underlying bug isn't fixed, it should be possible to have it working before the next release. And if that's the case, then it may be possible to get rid of all of your CTFE-only versions of functions in the near future.

@jmdavis
Copy link
Member

jmdavis commented Jul 25, 2011

Don has confirmed that bug 6374 will be fixed before the next release as well.

return lines;
}

// TODO: Remove this and use std.array.split when BUG6374 is fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

News flash: 6374 was recently fixed (dlang/dmd@20930500), and indeed .split is now CTFE-able.

@jmdavis
Copy link
Member

jmdavis commented Aug 14, 2011

Bug 3512 has been fixed.

@Abscissa
Copy link
Contributor Author

Ok, I've merged in the latest upstream Phobos changes and removed remaining ctfe_* functions. But I wasn't able to test it because of problems building the latest DMD/dsource/phobos. I think all I need is for DMD Pull Request #306 to be accepted ( dlang/dmd#306 ).

@jmdavis
Copy link
Member

jmdavis commented Aug 14, 2011

Well, you don't need the compiler test suite to pass to build the latest phobos. I gather that you're using DVM to do grab the latest dmd, and it needs the test suite to pass?

@Abscissa
Copy link
Contributor Author

Maybe it's a different problem I'm hitting then. I'm having trouble with mmfile just simply building phobos. My local repo of dmd, druntime and phobos were getting old, so I updated all of them with the latest master and tried to compile. Compiling druntime gave me this:

Internal error: backend\gflow.c 931

So I switched to 2.054 to compile druntime. That worked, but then when I moved on to phobos, I got this regardless of whether I used 2.054 or latest master:

std\mmfile.d(197): Error: no property 'Read' for type 'int'
std\mmfile.d(205): Error: no property 'ReadWriteNew' for type 'int'
std\mmfile.d(214): Error: no property 'ReadWrite' for type 'int'
std\mmfile.d(222): Error: no property 'ReadCopyOnWrite' for type 'int'

@jmdavis
Copy link
Member

jmdavis commented Aug 14, 2011

I don't know. I made sure that it all compiled before I create the pull request for it. However, David Simcha accidentally merged some changes from one of his branches into the main repository, and when he tried to undo it, he broke the build. So, the Phobos build is broken at the moment.

@Abscissa
Copy link
Contributor Author

God fucking dammit GitHub is fucking SLOW. Damn page even keeps hanging. I had to kill the browser twice before I could even type one word. And almost once more right before submitting. And when I'm at the text entry I have to type into notepad and then copy/paste into the page. Worthless fucking site. And the URLs and Back/Forward don't even fucking work. I'd switch back to JS-less FF2 but these fucking pull request pages don't even work with JS off. Can you believe it's actually taken me half an hour to post this fucking message? Goddam, how does anyone ever get anything done with this godawful excuse for a site? Fuck fuckeddy fuck fuck Fuck.

Ok, now that I have that out of my system:

Andrei: You're looking at an old commit, not the head of this pull request. I'll change the nested functions to be static, insert spaces after if/etc, and add superfluous braces, but please re-check the other stuff in head (probably with the "diff" button at top of this page). I think all of your other comments have already been fixed there.

@Abscissa
Copy link
Contributor Author

I'm blocked by issues #6509 and #6507

@Abscissa
Copy link
Contributor Author

Ok, #6509 and #6507 turned out to be invalid, but I am being blocked by #6516:

[2.055 beta] ICE: assert constfold.c(721) global.errors
http://d.puremagic.com/issues/show_bug.cgi?id=6516

@jmdavis
Copy link
Member

jmdavis commented Aug 27, 2011

For your information, I believe that the only formatting rules for Phobos that we actually follow as a group and require are

  1. One statement per line.
  2. Indent using spaces. No tabs.
  3. Indentation levels are 4 spaces each.
  4. Braces must be on their own line save for a few exceptions such as lambdas where they both end up on the same line.
  5. Lines have a soft limit of 80 characters and a hard limit of 120 characters. So, they should generally be within 80 characters, but can go longer. However, they can never exceed 120 characters.

From the changes in the "Use phobos's goofy formatting convention" commit, I get the impression that you think that other formatting guidelines exist, but they don't. Code within a function should be consistent, but spacing around parens and stuff like that is up to the individual programmer.

@Abscissa
Copy link
Contributor Author

I guess I may have misunderstood something. Andrei said up above:

"The code uses very different conventions than the usual in Phobos: two spaces indentation, no space after "if"/"while"/etc., no braces wherever possible, etc. Could you please change?"

I took that as meaning that space after foreach was expected and that braces wherever possible were expected.

@yebblies
Copy link
Member

It might be worth combining those commits before this is pulled. 17 commits for a ~200 line function seems a little excessive.

@jmdavis
Copy link
Member

jmdavis commented Aug 27, 2011

Well, Andrei likes to to put a single space after if, while, etc., but there is no requirement to do so, and much of Phobos does not. Some of the developers always use braces, but most do not I, believe. The indentation needs to be 4, and it needs to be spaces, but the other things mentioned there are debatable. No agreement has been made on requiring anything along those lines. So, you can do it either way. For instance, std.string is already doing the spacing on if, while, etc. both ways in a number of places. It's just usually consistent within a function.

And yes, you probably should rebase this so that it's fewer commits.

@Abscissa
Copy link
Contributor Author

Abscissa commented Sep 2, 2011

Merged all of this into one commit:
#228

@Abscissa Abscissa closed this Sep 2, 2011
kuettler pushed a commit to kuettler/phobos that referenced this pull request Feb 6, 2018
Remove findtags in favor of using chmgen-emitted tags
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants